Skip to content

Developed task#5419

Open
LittleMazur wants to merge 3 commits into
mate-academy:masterfrom
LittleMazur:develop
Open

Developed task#5419
LittleMazur wants to merge 3 commits into
mate-academy:masterfrom
LittleMazur:develop

Conversation

@LittleMazur
Copy link
Copy Markdown

  • DEMO LINK

  • TEST REPORT LINK

  • Icon implemented using background-image CSS property

  • Inputs are written inside of 'form' tag with correctly passed attributes

  • All Typical Mistakes from BEM lesson theory are checked.

  • Code follows all the Code Style Rules ❗️

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

This implementation has critical violations of task requirements that must be fixed before approval. The most significant issues are: (1) Search bar containers use <div> instead of <form> tags, violating checklist item #6 and the explicit requirement that "Inputs are written inside of 'form' tag"; (2) The data-qa="big" and data-qa="small" attributes are placed on <input> elements when they should be on the parent <form> elements; (3) The required data-qa="keypress" attribute is missing from the big search input; (4) Search bars don't stretch to full width — add width: 100% to .search-bar__item class; (5) Font-family declaration is missing a fallback font. Additionally, visual regression tests are failing with 4-5% differences, likely caused by these structural issues. Please fix all five data-qa related problems first, then ensure search bars have width: 100%, and add a fallback font like Arial, sans-serif to the font-family.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/style.css Outdated
src: url('fonts/Avenir-Heavy.ttf') format('truetype');
font-weight: 900;
font-style: normal;
font-display: swap;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checklist violation: 'Inputs are written inside of 'form' tag' - The inputs are inside a <div> instead of <form> tags. Each search bar should be wrapped in a <form> element with the appropriate data-qa attributes.

Comment thread src/style.css Outdated
Comment on lines +21 to +26
}

.search-bar {
display: flex;
flex-direction: column;
margin-top: 20px;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Task requirement violation: 'add attribute data-qa="keypress" to input in big search form' - This attribute is missing from the big input element.

Comment thread src/style.css
Comment on lines +29 to +33

.search-bar__item {
box-shadow: 0 1px 8px rgba(61, 78, 97, 0.1);
border: 1px #e1e7ed solid;
color: #3d4e61;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Task requirement violation: 'search bar must stretch the full width' - Inputs are missing width: 100% property. Add width: 100% to .search-bar__item class to make them stretch to full width.

Comment thread src/style.css Outdated
Comment on lines +2 to +3
body {
font-family: Avenir, sans-serif;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checklist item #12 violation: 'Remember to use fallback fonts' - The font-family declaration should include a fallback. Consider: font-family: 'Avenir', Arial, sans-serif;

Comment thread src/index.html Outdated
data-qa="keypress"
placeholder="Try “Los Angeles“"
/>
<div class="search-bar">
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item #6 (Use semantic tags) and the requirement 'Inputs are written inside of form tag'. Use <form> instead of <div> for the container.

Comment thread src/index.html Outdated
Comment on lines +28 to +32
<input
type="text"
data-qa="small"
placeholder="Try “Los Angeles“"
class="search-bar__item search-bar__item--small"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data-qa="small" attribute should be on the <form> element, not on <input>. According to requirements: 'add attribute data-qa="small" for small search form'.

Comment thread src/index.html Outdated
Comment on lines +21 to +25
<input
type="text"
data-qa="big"
placeholder="Try “Los Angeles“"
class="search-bar__item search-bar__item--big"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing required attribute data-qa="keypress" on this input. Requirements state: 'add attribute data-qa="keypress" to input in big search form'.

Comment thread src/index.html
Comment on lines +22 to 27
type="text"
data-qa="big"
placeholder="Try “Los Angeles“"
class="search-bar__item search-bar__item--big"
/>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Search bar doesn't stretch to full width. Requirement states 'search bar must stretch the full width'. Add width: 100% to make it stretch.

Comment thread src/index.html Outdated
Comment on lines +21 to +25
<input
type="text"
data-qa="big"
placeholder="Try “Los Angeles“"
class="search-bar__item search-bar__item--big"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The input element with data-qa="big" is missing the required name attribute. Forms should have correctly passed attributes including a name for the input.

Comment thread src/index.html Outdated
Comment on lines +28 to +32
<input
type="text"
data-qa="small"
placeholder="Try “Los Angeles“"
class="search-bar__item search-bar__item--small"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The input element with data-qa="small" is missing the required name attribute. Forms should have correctly passed attributes including a name for the input.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

This is an educational approval to help you progress while learning. Great job implementing both search bars with proper form structure, data-qa attributes, and the 20px gap between them. The hover and focus states, font fallbacks, and BEM naming convention are all well done. However, there are two things to address: remove the unnecessary data-qa="hover" attribute from the small input (only data-qa="keypress" on the big input was required), and replace the * selector for box-sizing: border-box in your CSS with a more specific approach like applying it directly to form elements. Also, keep an eye on the 80-character line limit for the viewport meta tag in index.html. These are minor fixes that will make your code more professional and performant.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread src/index.html
Comment on lines +38 to +42
>
<input
class="form__input form__input--small"
name="search"
type="text"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data-qa="hover" attribute on the small search input is not required by the task specification. The task only specifies data-qa="keypress" for the big search input. This extra attribute may cause test failures.

Comment thread src/style.css
Comment on lines +29 to +30

box-sizing: border-box;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses the * selector which violates checklist item #17: "Don't use * selector (it impacts performance)". Consider using html { box-sizing: border-box; } *, *::before, *::after { box-sizing: inherit; } approach or apply box-sizing directly to form elements instead.

Comment thread src/style.css
Comment on lines +5 to +7
}

@font-face {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line exceeds 80 characters, violating checklist item #8. The viewport content attribute is too long. Consider breaking it into multiple lines for better readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants