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

add task solution #4745

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add task solution #4745

wants to merge 1 commit into from

Conversation

vkysh
Copy link

@vkysh vkysh commented Feb 1, 2025

  • 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

@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 job on your submission! 🎉 Your code meets most of the task requirements, and I'm approving it to help you move forward with your learning journey. However, there are a few areas to improve:

  1. Font Weight: Ensure the default font-weight is set to 300 as required.
  2. Image Paths: Verify that images are correctly placed in the src/images directory.
  3. HTML Formatting: Maintain correct attribute formatting and use semantic tags for better accessibility and SEO.
  4. CSS Selectors: Use classes instead of tag names for styling, except for html and body.
  5. Fallback Fonts: Include fallback fonts in your CSS.
  6. Accessibility: Add <label> elements to input fields for better accessibility.
  7. CSS Consistency: Use CSS variables for repeated values like colors and ensure consistent units.

Addressing these points will enhance the quality and maintainability of your code. Keep up the good work and continue learning from these insights! 🚀

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

Comment on lines +27 to +32
<input
class="search-bar__input search-bar__input--big"
type="text"
data-qa="keypress"
placeholder="Try “Los Angeles“"
/>

Choose a reason for hiding this comment

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

Consider adding a <label> element for the input field to improve accessibility. This helps screen readers to associate the input with its purpose.

Comment on lines +43 to +47
<input
class="search-bar__input search-bar__input--small"
type="text"
placeholder="Try “Los Angeles“"
/>

Choose a reason for hiding this comment

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

Similar to the previous form, consider adding a <label> element for this input field to enhance accessibility.

.search-bar__input {
width: 100%;
box-sizing: border-box;
border: 1px solid #e1e7ed;

Choose a reason for hiding this comment

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

Consider using CSS variables for colors like #e1e7ed to maintain consistency and make future changes easier.

box-shadow: 2px 2px 5px rgba(0, 0, 0, 0.1);

font-family: 'Avenir Book', sans-serif;
color: #3d4e61;

Choose a reason for hiding this comment

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

Similarly, using CSS variables for colors like #3d4e61 can help maintain consistency across the stylesheet.

}

.search-bar__input:hover {
box-shadow: 0 4px 4px 0 rgba(0, 0, 0, 0.25);

Choose a reason for hiding this comment

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

Ensure consistent use of units. Here, 0 does not require a unit, but for consistency, you might want to use 0px if other similar properties use px.

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