add task solution #4545
add task solution #4545
Conversation
There was a problem hiding this comment.
Great job so far! However, there are a few issues that need to be addressed, especially with regards to class naming and the use of semantic tags. Keep up the good work, and with a few tweaks, your code will be top-notch! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| @@ -11,22 +11,38 @@ | |||
| content="ie=edge" | |||
| /> | |||
| <title>Document</title> | |||
There was a problem hiding this comment.
It is advisable to give your HTML document a more descriptive title, rather than the default 'Document'. This is important for SEO and for better understanding of the page content.
src/index.html
Outdated
| > | ||
| <label> | ||
| <input | ||
| class="search-bar search-bar__big" |
There was a problem hiding this comment.
It looks like you have two classes assigned to the same element. If 'search-bar' is a block, it should not be a child of itself ('search-bar__big'). Please consider revising your class naming to adhere to BEM methodology.
src/index.html
Outdated
| > | ||
| <label> | ||
| <input | ||
| class="search-bar search-bar__small" |
There was a problem hiding this comment.
Same issue here as in line 27. Please revise your class naming.
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </label> |
There was a problem hiding this comment.
As per HTML Content checklist, it's recommended to use semantic tags where possible. In this case, 'label' tag is used without a 'for' attribute and without any text, which doesn't provide any semantic value. Consider removing the 'label' tag if it's not necessary, or provide a 'for' attribute linking to the 'id' of the input field.
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </label> |
There was a problem hiding this comment.
Same issue here as in line 32. Please revise your usage of 'label' tag.
src/style.css
Outdated
| } | ||
|
|
||
| body { | ||
| font-family: 'Avenir Book', sans-serif; |
There was a problem hiding this comment.
You have used a fallback font for the body tag, which is good practice. However, you should consider adding more fallback fonts. For instance, you can add 'Arial', 'Helvetica' as fallbacks. This will ensure that if 'Avenir Book' doesn't load, the browser will try to load 'Arial', then 'Helvetica', and finally 'sans-serif'.
| } | ||
|
|
||
| .search-bar { | ||
| font-family: Avenir, sans-serif; |
There was a problem hiding this comment.
You have defined the font-family for the class 'search-bar' as 'Avenir'. However, you have not defined a font-face for 'Avenir'. You should either define a font-face for 'Avenir' or use one of the fonts you have defined, like 'Avenir Book' or 'Avenir Heavy'.
| font-family: Avenir, sans-serif; | ||
| width: 100%; | ||
| display: block; | ||
| margin-top: 20px; |
There was a problem hiding this comment.
You have added a top margin to the 'search-bar' class. This is not a good practice as per the checklist. Instead of adding margins to the top and bottom, it is recommended to add them to either the top or the bottom to avoid potential margin collapse.
| } | ||
|
|
||
| .search-bar__big { | ||
| height: 70px; |
There was a problem hiding this comment.
You have fixed the height of the 'search-bar__big' class. Unless there is a requirement, it is recommended not to fix the container size. Let the content size dictate it to avoid overflow or accidental scroll bars.
| } | ||
|
|
||
| .search-bar__small { | ||
| height: 42px; |
There was a problem hiding this comment.
Similar to the 'search-bar__big' class, you have also fixed the height of the 'search-bar__small' class. It is recommended not to fix the container size unless there is a requirement.
There was a problem hiding this comment.
You've done a really good job, but there are a few things you need to polish up. Don't forget to add fallback fonts in your CSS for better compatibility and make your 'title' tag more descriptive for SEO purposes. Also, please fix the 'for' attribute in the label element to match the 'id' of its associated input for accessibility. Keep it up, you're almost there! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| action="#" | ||
| data-qa="big" | ||
| > | ||
| <label for="search-input"> |
There was a problem hiding this comment.
The 'for' attribute of the label element must be equal to the id of a non-hidden form control. Please add an 'id' attribute to the input and make sure it matches with 'for' attribute of the label. This is an accessibility issue.
src/index.html
Outdated
| action="#" | ||
| data-qa="small" | ||
| > | ||
| <label for="search-input"> |
There was a problem hiding this comment.
The 'for' attribute of the label element must be equal to the id of a non-hidden form control. Please add an 'id' attribute to the input and make sure it matches with 'for' attribute of the label. This is an accessibility issue.
src/index.html
Outdated
| @@ -11,22 +11,38 @@ | |||
| content="ie=edge" | |||
| /> | |||
| <title>Document</title> | |||
There was a problem hiding this comment.
The 'title' tag should be more descriptive about the content of the page. This is an SEO issue.
src/style.css
Outdated
| } | ||
|
|
||
| body { | ||
| font-family: Avenir, sans-serif; |
There was a problem hiding this comment.
According to the checklist, remember to use fallback fonts - alternative font-family in case the main one doesn't work. You should provide a list of similar, fallback fonts that can be used if the user's computer doesn't have the first font specified. For example, you can modify this line to: font-family: Avenir, Arial, sans-serif;
There was a problem hiding this comment.
Great job on your work! There are just a few areas that could use some improvement. Remember to include a fallback font in your CSS file and avoid setting a fixed width to prevent overflow issues. Also, ensure your HTML attributes are correctly formatted and indented for better readability. Keep up the good work and continue learning! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <form | ||
| action="#" | ||
| data-qa="big" | ||
| > | ||
| <label for="search-input"> | ||
| <input | ||
| id="search-input" | ||
| class="search-bar search-bar--big" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </label> | ||
| </form> |
There was a problem hiding this comment.
According to the checklist, you should add empty lines between multiline sibling blocks of HTML. This will improve readability.
| <label for="search-input"> | ||
| <input | ||
| id="search-input" | ||
| class="search-bar search-bar--big" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </label> |
There was a problem hiding this comment.
Remember about correct indentation between parent and child elements. Each level of nesting, including text, contained inside the element, requires 2-space offset. Also blank line shouldn't be between parent and child elements.
| <input | ||
| id="search-input" | ||
| class="search-bar search-bar--big" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> |
There was a problem hiding this comment.
Keep your attributes correctly formatted. If the HTML-element has long attribute values or number of attributes is more than 2 - start each one, including the first, on the new line with 2-space indentation related to tag. Tag’s closing bracket should be on the same level as opening one.
| } | ||
|
|
||
| body { | ||
| font-family: Avenir, Arial, Helvetica, sans-serif; |
There was a problem hiding this comment.
Remember to use fallback fonts - alternative font-family in case the main one doesn't work. You can add 'Arial, Helvetica, sans-serif' as fallback fonts for 'Avenir'.
| } | ||
|
|
||
| .search-bar { | ||
| font-family: Avenir, sans-serif; |
There was a problem hiding this comment.
Remember to use fallback fonts - alternative font-family in case the main one doesn't work. You can add 'Arial, Helvetica, sans-serif' as fallback fonts for 'Avenir'.
|
|
||
| .search-bar { | ||
| font-family: Avenir, sans-serif; | ||
| width: 100%; |
There was a problem hiding this comment.
Avoid fixing container size if there is no such requirement. Let the content size dictate it. To avoid overflow or accidental scroll bar.
DEMO LINK
TEST REPORT LINK