im trying #4540
im trying #4540
Conversation
There was a problem hiding this comment.
Great job on your code! 😊 However, I would recommend making a few improvements before I can approve it. For instance, place the transition property in the original selector, not just in the hover or focus state. Also, consider using more semantic HTML tags and meaningful class names. Don't worry, these are common areas for improvement and you're doing well! Keep it up! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| <form class="form"> | ||
| <div | ||
| class="form__icon form__icon--big" | ||
| data-qa="big" | ||
| > | ||
| <input | ||
| class="input input__big" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </div> | ||
|
|
||
| <input | ||
| type="text" | ||
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| <div | ||
| class="form__icon form__icon--small" | ||
| data-qa="small" | ||
| > | ||
| <input | ||
| type="text" | ||
| placeholder="Try “Los Angeles“" | ||
| class="input input__small" | ||
| /> | ||
| </div> | ||
| </form> |
There was a problem hiding this comment.
It's recommended to use semantic HTML tags where possible. For example, the form could be wrapped in a section tag, which would give it more semantic meaning. Semantic tags help with accessibility and SEO.
src/index.html
Outdated
| <input | ||
| class="input input__big" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> |
There was a problem hiding this comment.
The class names 'input__big' and 'input__small' are not very descriptive. It's better to use class names that describe the meaning of the element, not its appearance. For example, if these inputs are for search, you could use 'search-input' instead.
src/index.html
Outdated
| <input | ||
| type="text" | ||
| placeholder="Try “Los Angeles“" | ||
| class="input input__small" | ||
| /> |
There was a problem hiding this comment.
The input elements are missing the 'name' attribute. This attribute is important for form submission and accessibility.
src/style.css
Outdated
| font-weight: 300; | ||
| src: | ||
| url(../src/fonts/Avenir-Book.ttf) format('truetype'), | ||
| url(../src/fonts/Avenir-Heavy.ttf) format('truetype'); |
There was a problem hiding this comment.
It's better to split your font-face declarations into two separate ones for different font weights. It's a common practice to have a separate @font-face declaration for each font weight and style combination. This way, the browser can select the correct font file based on the font-weight and font-style used in your CSS rules.
src/style.css
Outdated
| border: 1px solid #e1e7ed; | ||
| border-radius: 4px; | ||
| color: #3d4e61; | ||
| font-family: Avenir, Arial, 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. If the Avenir font fails to load for some reason, your text will be displayed using the default browser font, which may not look as expected. It's a good practice to specify a list of fallback fonts that should be used if the primary font is not available. You can specify a list of fonts in order of preference, separated by commas. After Arial, you can add 'sans-serif' as a generic fallback.
src/style.css
Outdated
| } | ||
|
|
||
| body { | ||
| margin: 0 8px; |
There was a problem hiding this comment.
Be consistent with your margins. It's better to add only top or bottom margins, but not both, to avoid potential margin collapse. In your case, you've added both vertical and horizontal margins to the body. It's not a critical issue in this case, but it's a good practice to follow.
src/style.css
Outdated
|
|
||
| .input__big:hover { | ||
| box-shadow: 0 4px 4px #3d4e6133; | ||
| transition: box-shadow 0.3s; |
There was a problem hiding this comment.
The transition property should be placed in the original selector, not in the hover or focus state. It's a common practice to include the transition property in the original class, so that the transition is smooth both ways - when the pseudo-class is activated and when it's deactivated.
src/style.css
Outdated
|
|
||
| .input__small:hover { | ||
| box-shadow: 0 4px 4px #3d4e6133; | ||
| transition: box-shadow 0.3s; |
There was a problem hiding this comment.
The transition property should be placed in the original selector, not in the hover state. Refer to the comment above for more details.
src/style.css
Outdated
| .input__big:focus { | ||
| text-shadow: 0 3px 3px #bbbec2; | ||
| box-shadow: 0 4px 4px #3d4e6133; | ||
| transition: box-shadow 0.3s; |
There was a problem hiding this comment.
The transition property should be placed in the original selector, not in the focus state. Refer to the comment above for more details.
src/style.css
Outdated
| .input__small:focus { | ||
| text-shadow: 0 3px 3px #bbbec2; | ||
| box-shadow: 0 4px 4px #3d4e6133; | ||
| transition: box-shadow 0.3s; |
There was a problem hiding this comment.
The transition property should be placed in the original selector, not in the focus state. Refer to the comment above for more details.
There was a problem hiding this comment.
Great work on this project! 👏 Your HTML and CSS code are generally well-structured and follow good practices. However, there are a few areas that could be improved. Please review the comments and suggestions made on your code to help you improve your future projects. Remember, the aim is to help you learn and grow. Keep coding and keep improving! 👍🎉
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| <body class="body"> |
There was a problem hiding this comment.
The class name body doesn't represent the meaning of the content. Consider using a more descriptive class name that represents the role or purpose of the element in the context of the interface.
| action="#" | ||
| > | ||
| <input | ||
| class="search_input_big form" |
There was a problem hiding this comment.
The class name search_input_big form could be improved. Avoid using tag names in class names (like form). Also, avoid specifying styles in class names (like big). Instead, consider something like search-input search-input--large.
| action="#" | ||
| > | ||
| <input | ||
| class="search_input_small form" |
There was a problem hiding this comment.
The class name search_input_small form could be improved. Avoid using tag names in class names (like form). Also, avoid specifying styles in class names (like small). Instead, consider something like search-input search-input--small.
| <input | ||
| class="search_input_big form" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> |
There was a problem hiding this comment.
The input elements are missing the name attribute. This attribute is important for accessibility and for data submission.
| > | ||
| <input | ||
| class="search_input_small form" | ||
| type="text" | ||
| placeholder="Try “Los Angeles“" | ||
| /> |
There was a problem hiding this comment.
The input elements are missing the name attribute. This attribute is important for accessibility and for data submission.
| } | ||
|
|
||
| .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. It is usually a good practice to provide a list of similar fonts that the browser can fall back to in case the primary font fails to load. This ensures that your website will still look good, even if the font files are not available for some reason.
| } | ||
|
|
||
| .body { | ||
| font-family: Avenir, Arial, Helvetica, sans-serif; |
There was a problem hiding this comment.
Don't use tag names for styling (except html and body). It's better to style all elements using .classes and if needed with :pseudo-classes, pseudo-elements and [attributes]. In your case, .body and .form look like tag selectors, but they are actually class selectors. It's a bit confusing and it's better to avoid such class names.
| .form { | ||
| font-family: Avenir, Arial, Helvetica, sans-serif; | ||
| display: block; | ||
| margin-top: 20px; |
There was a problem hiding this comment.
Be consistent with your margins (Add only top or bottom, but not both). To avoid potential margin collapse. Here you've added a margin-top to your .form element. Make sure that you're not adding margin-bottom to the elements above this one.
| border-radius: 4px; | ||
| box-shadow: 0 1px 8px 0 #3d4e611a; | ||
| width: 100%; | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
Don't fix container size (if there is no such a requirement). Let the content size dictate it. To avoid overflow or accidental scroll bar. Here, you have set the width of the .form element to 100%. This is fine if you want the form to take up the full width of its parent, but be careful not to set fixed heights on containers unless necessary, as this can lead to overflow issues.
No description provided.