solution#5423
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has critical issues that violate the task requirements. The most important fix: Both search bars must be wrapped in <form> tags instead of <div> elements, as the checklist explicitly requires 'Inputs are written inside of form tag with correctly passed attributes'. Move the data-qa="big" and data-qa="small" attributes from their current locations to the form elements, and remove data-qa="hover" from the small input (only data-qa="keypress" was specified for the big input's input element). Add width: 100% to the .search class so search bars stretch to full width as required. Finally, fix the gap between search bars to be exactly 20px by removing margin-top from the small search bar and adding margin-bottom: 20px to the large search bar instead. The visual regression tests are failing (1-3% difference across all scenarios) which confirms the CSS styling doesn't match the expected design specifications.
✨ 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
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| <input |
There was a problem hiding this comment.
The input is missing the name attribute which is typically required for form submissions. Consider adding appropriate name attributes for both search inputs.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is close to completion but has one critical missing requirement. The big search bar must have a 20px top indent as specified in the requirements — the wrapper <div> currently has no top spacing, causing the search bar to sit directly at the top of the page. Add padding-top: 20px to the wrapper div or margin-top: 20px to the big search form. This is also causing visual regression tests to fail with 6%+ differences. Additionally, fix the @font-face syntax by adding quotes around the format values: format('ttf') instead of format(ttf). The form wrapping, data-qa attributes, and search bar styling are otherwise correctly implemented.
✨ 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
| .search { | ||
| box-sizing: border-box; | ||
| border: 1px solid rgba(225, 231, 237, 1); | ||
| border-radius: 4px; |
There was a problem hiding this comment.
The big search bar must have top indent 20px (requirement: 'the big search bar must have top indent 20px'). Add padding-top: 20px to this wrapper div.
| @@ -17,16 +17,33 @@ | |||
| /> | |||
There was a problem hiding this comment.
The requirement 'the big search bar must have top indent 20px' is not implemented. Add margin-top: 20px to the body or .search--large class to create this top spacing.
3 participants
DEMO LINK