-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
implement two search bars #4624
base: master
Are you sure you want to change the base?
Conversation
galina-palyukh
commented
Nov 28, 2024
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! 😊 It looks like there are a few critical issues that need to be addressed before we can approve this pull request.
-
HTML Form Action: The
action
attribute of theform
element is set to an empty string, which means the form won't submit data to any server endpoint. Please ensure you specify a valid URL or endpoint for the form submission. -
HTML Data Attributes: The
data-qa
attributes for the big and small search forms are incorrect. They should bedata-qa="big"
anddata-qa="small"
respectively, but are currently set todata-qa="keypress"
anddata-qa="hover"
. -
CSS Font Weights: There is redundancy in the
@font-face
declarations for the 'Avenir' font. Ensure that different font files are used for different weights, or confirm that the font file supports multiple weights. -
CSS Styling: The default
font-weight
should be 300, but the placeholder is set to 400. Additionally, styles for thehover
andfocus
states of the search bar are missing. -
Image Usage: The task requires using images from
src/images
, but there is no evidence of this in the provided files.
These issues are important to ensure the functionality and requirements of the task are fully met. Once these are addressed, we can move forward with the approval. Keep up the great work, and feel free to reach out if you need any help! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review
button in the Reviewers tab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great effort on updating the HTML and CSS files! 🎉 Here are some key points to focus on for improvement:
-
HTML Form Action: The
action
attribute is set to "/", which might not be the intended endpoint. Ensure it points to the correct endpoint for form submission. -
HTML Data Attributes: The
data-qa
attributes are mostly correct, but the big search form input is missing thedata-qa="keypress"
attribute. Make sure to add it. -
CSS Font Weights: The placeholder's default
font-weight
should be 300, not 400. Also, ensure thehover
state font-weight is set to 300. -
Image Usage: Good job using images from
src/images
for the background of input fields.
These adjustments will help align your code with the task requirements. Remember, addressing these issues will enhance your skills and ensure your code functions as intended. Keep up the good work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨