Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

galina-palyukh
Copy link

Copy link

@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.


Hello! 😊 It looks like there are a few critical issues that need to be addressed before we can approve this pull request.

  1. HTML Form Action: The action attribute of the form 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.

  2. HTML Data Attributes: The data-qa attributes for the big and small search forms are incorrect. They should be data-qa="big" and data-qa="small" respectively, but are currently set to data-qa="keypress" and data-qa="hover".

  3. 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.

  4. CSS Styling: The default font-weight should be 300, but the placeholder is set to 400. Additionally, styles for the hover and focus states of the search bar are missing.

  5. 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.

src/index.html Outdated Show resolved Hide resolved
src/style.css Outdated Show resolved Hide resolved
Copy link

@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.

Great effort on updating the HTML and CSS files! 🎉 Here are some key points to focus on for improvement:

  1. 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.

  2. HTML Data Attributes: The data-qa attributes are mostly correct, but the big search form input is missing the data-qa="keypress" attribute. Make sure to add it.

  3. CSS Font Weights: The placeholder's default font-weight should be 300, not 400. Also, ensure the hover state font-weight is set to 300.

  4. 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! ✨

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.

2 participants