Develop #4531
Develop #4531
Conversation
michelletani
commented
Oct 10, 2024
michelletani
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Hey there! Great job on the HTML file. It's well-structured and nicely formatted. However, there are a few changes needed. The 'input' elements are missing the 'name' attribute which is important for form submission and accessibility. Also, consider using more descriptive class and title names. Keep up the good work and happy coding! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
There was a problem hiding this comment.
Great job so far! 😊 However, there are a few issues that need to be addressed before we can merge this PR. Please, add the 'id' and 'name' attributes to your input fields for better accessibility and data handling. Also, consider changing the title of the page to something more descriptive. Keep up the good work! 💪
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
There was a problem hiding this comment.
Despite some minor issues, the pull request is approved. The HTML file is generally well-structured and follows the basic HTML structure. However, there are areas for improvement, such as avoiding the use of the * selector in your CSS for better performance and using label elements for your input fields to improve accessibility. Also, consider making the title of your HTML document more descriptive. Keep up the good work and continue to improve! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <input | ||
| class="search-bars search-bars-big" | ||
| type="text" | ||
| data-qa="keypress" | ||
| data-qa="big" | ||
| placeholder="Try “Los Angeles“" | ||
| /> |
There was a problem hiding this comment.
The input element here has no associated label element. For accessibility purposes, it's recommended to use a label with a for attribute that matches the id of the input. It would make it more accessible for people using screen readers and also improves the user experience for those who can see the screen because clicking on the label will focus the input.
| <input | ||
| class="search-bars search-bars-small" | ||
| type="text" | ||
| data-qa="hover" | ||
| data-qa="small" | ||
| placeholder="Try “Los Angeles“" | ||
| /> |
There was a problem hiding this comment.
The same comment about the label applies to this input element as well.
| @font-face { | ||
| font-family: Avenir; | ||
| src: url('fonts/Avenir-Book.ttf') format('truetype'); | ||
| font-weight: 300; | ||
| } |
There was a problem hiding this comment.
You've done a good job of defining the @font-face, but you forgot to include a fallback font family. This is important for cases where the main font doesn't load correctly. You can add a generic font family like sans-serif as a fallback.
| } | ||
|
|
||
| .search-bars::placeholder { | ||
| font-family: Avenir, sans-serif; |
There was a problem hiding this comment.
Great job on using a fallback font here. This ensures that if the 'Avenir' font fails to load for some reason, the browser will use a generic 'sans-serif' font instead.
| .search-bars { | ||
| box-sizing: border-box; | ||
| display: flex; | ||
| width: 100%; | ||
| margin-top: 20px; | ||
| background: #fff; | ||
| border: 1px solid #e1e7ed; | ||
| border-radius: 4px; | ||
| box-shadow: 0 1px 8px 0 #3d4e611a; | ||
|
|
||
| background-image: url(images/Search.svg); | ||
| background-repeat: no-repeat; | ||
| } |
There was a problem hiding this comment.
In your .search-bars class, you're using the * selector. This can impact performance as it applies the style to all elements. It's better to set styles only for elements that require them. Also, you're applying a top margin. It's a good practice to be consistent with your margins. Apply either top or bottom, but not both, to avoid potential margin collapse.
| .search-bars:hover { | ||
| box-shadow: 0 3px 8px 0 #3d4e6133; | ||
| } |
There was a problem hiding this comment.
In your .search-bars:hover class, you're using the * selector. This can impact performance as it applies the style to all elements. It's better to set styles only for elements that require them.
| .search-bars:focus { | ||
| background-image: url(images/Search.svg), | ||
| linear-gradient(180deg, #fff 0%, #f6f6f7 100%); | ||
| box-shadow: 0 3px 8px 0 #3d4e6133; | ||
| border-radius: 3px; | ||
| border: 1px solid #e1e7ed; | ||
| outline: none; | ||
| } |
There was a problem hiding this comment.
In your .search-bars:focus class, you're using the * selector. This can impact performance as it applies the style to all elements. It's better to set styles only for elements that require them.
| .search-bars-big { | ||
| height: 70px; | ||
| padding-left: 62px; | ||
| font-size: 16px; | ||
| background-position: | ||
| 25px 24px, | ||
| 0 0; | ||
| background-size: | ||
| 19px, | ||
| 100% 100%; | ||
| } |
There was a problem hiding this comment.
In your .search-bars-big class, you're using the * selector. This can impact performance as it applies the style to all elements. It's better to set styles only for elements that require them.
| .search-bars-small { | ||
| height: 42px; | ||
| padding-left: 33px; | ||
| font-size: 14px; | ||
| background-position: | ||
| 12px 14px, | ||
| 0 0; | ||
| background-size: | ||
| 11px, | ||
| 100% 100%; | ||
| } |
There was a problem hiding this comment.
In your .search-bars-small class, you're using the * selector. This can impact performance as it applies the style to all elements. It's better to set styles only for elements that require them.
