Develop #4482
Develop #4482
Conversation
iirk1
commented
Sep 23, 2024
iirk1
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
While you've done a great job with the Pull Request, there are a few areas that need adjustments to ensure the code meets all the requirements. Specifically, we need to address the fixed sizes for .search-box variants and ensure proper font handling. Don't worry, these are important learning steps on your coding journey! 🛠️💡
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <div class="search-box search-box--big"> | ||
| <input | ||
| class="search-txt search-txt--big" | ||
| type="text" | ||
| data-qa="big" | ||
| placeholder="Try “Los Angeles“" | ||
| /> |
There was a problem hiding this comment.
Add empty lines between multiline sibling blocks of HTML to simplify reading.
src/style.css
Outdated
| @@ -1 +1,90 @@ | |||
| /* add styles here */ | |||
| body { | |||
| font-family: ' Avenir ', sans-serif; | |||
There was a problem hiding this comment.
The font-family declaration should include a fallback font. It's a good practice to have a generic font family specified in case the primary font fails to load. For example: font-family: 'Avenir', Arial, sans-serif;.
src/style.css
Outdated
| @@ -1 +1,90 @@ | |||
| /* add styles here */ | |||
| body { | |||
| font-family: ' Avenir ', sans-serif; | |||
There was a problem hiding this comment.
There are extra spaces inside the quotes of the font-family name 'Avenir'. It should be written without leading or trailing spaces as font-family: 'Avenir', sans-serif;.
src/style.css
Outdated
| @font-face { | ||
| font-family: ' Avenir '; | ||
| src: url('./fonts/Avenir-Book.ttf') format('truetype'); | ||
| } |
There was a problem hiding this comment.
The @font-face rule should also include a font-weight property to specify the weight of the font being defined. This helps in properly selecting the right font when different weights are used in the CSS.
| .search-box { | ||
| position: relative; | ||
| margin: 20px 0; | ||
| box-shadow: 0 1px 8px 0 #3d4e611a; | ||
| border-radius: 4px; |
There was a problem hiding this comment.
The .search-box class should not fix the size of the container. Instead, it should allow the content to dictate the size. Make sure there is no requirement for a fixed size before applying this change.
| } | ||
|
|
||
| .search-box--big { | ||
| height: 70px; |
There was a problem hiding this comment.
Ensure that there is no requirement for a fixed size for .search-box--big. If the content can dictate the size, avoid setting a fixed height.
| } | ||
|
|
||
| .search-box--small { | ||
| height: 42px; |
There was a problem hiding this comment.
Ensure that there is no requirement for a fixed size for .search-box--small. If the content can dictate the size, avoid setting a fixed height.
There was a problem hiding this comment.
Hey there! Your Pull Request looks pretty solid, but there are a few tweaks that need your attention. The use of quotes around the URL in your CSS and the correct formatting of your HTML attributes are crucial for browser compatibility and readability. Also, make sure to match the font-family names exactly and keep the CSS and HTML clean and consistent. These changes will make your code shine even brighter! ✨ Keep up the good work, and let's polish these few spots for an even better result! 🛠️
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <div class="search-box search-box--big"> | ||
| <input | ||
| class="search-txt search-txt--big" | ||
| type="text" | ||
| data-qa="big" | ||
| placeholder="Try “Los Angeles“" | ||
| /> |
There was a problem hiding this comment.
Keep your attributes correctly formatted. Since the input element on lines 21-26 has more than 2 attributes, each attribute should start on a new line with proper indentation.
| <div class="search-box search-box--small"> | ||
| <input | ||
| class="search-txt search-txt--small" | ||
| type="text" | ||
| data-qa="small" | ||
| placeholder="Try “Los Angeles“" | ||
| /> |
There was a problem hiding this comment.
Keep your attributes correctly formatted. The input element on lines 30-35 should have each attribute on a new line with the correct indentation.
| data-qa="big" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Add empty lines between multiline sibling blocks of HTML. There should be an empty line between the div elements on lines 20-27 and 29-36.
src/style.css
Outdated
| @@ -1 +1,90 @@ | |||
| /* add styles here */ | |||
| body { | |||
| font-family: ' Avenir', Arial, sans-serif; | |||
There was a problem hiding this comment.
There's an unnecessary space before 'Avenir' in the font-family declaration. It should be removed to maintain clean and consistent code formatting.
src/style.css
Outdated
| } | ||
|
|
||
| @font-face { | ||
| font-family: ' Avenir', sans-serif; |
There was a problem hiding this comment.
Similar to the previous comment, remove the unnecessary space before 'Avenir' in the font-family declaration within the @font-face rule.
| font-weight: 300; | ||
| } | ||
|
|
||
| .search-box { |
There was a problem hiding this comment.
The selector '.search-box' is styling a class name directly. According to the checklist, you should avoid using tag names for styling (except 'html' and 'body'). It's recommended to use classes for styling elements.
| height: 42px; | ||
| } | ||
|
|
||
| .search-box--big::before { |
There was a problem hiding this comment.
The image URL in the background-image property should be enclosed in single or double quotes for consistency and to ensure compatibility across different browsers.
| height: 19px; | ||
| } | ||
|
|
||
| .search-box--small::before { |
There was a problem hiding this comment.
The image URL in the background-image property should be enclosed in single or double quotes for consistency and to ensure compatibility across different browsers.
| } | ||
|
|
||
| .search-txt { | ||
| border-radius: 4px; |
There was a problem hiding this comment.
The '.search-txt' class is applying a border-radius, which is not needed here since this class is used for text elements and not for any box/container elements.
| } | ||
|
|
||
| .search-txt:focus { | ||
| font-weight: 900; |
There was a problem hiding this comment.
Applying a text-shadow with a large spread on focus can be visually straining for users. Consider a more subtle effect or remove it if it's not necessary for the design.
There was a problem hiding this comment.
Your pull request has been approved to encourage your progress! 🌟 However, please review the following points to improve your code:
- Implement a comprehensive set of
@font-facerules for better text rendering. - Be consistent with your margin conventions to avoid layout issues.
- Avoid setting fixed container sizes when not required for better responsiveness.
- Always include fallback fonts to ensure text is displayed even if the primary font fails.
- Format your attributes correctly, with each attribute on a new line and proper indentation.
- Separate multiline sibling HTML blocks with empty lines for better readability.
- Use 2 spaces for indentation to maintain code clarity.
Keep up the good work, and consider addressing these points in future updates to enhance your project! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <div class="search-box search-box--big"> | ||
| <input | ||
| class="search-txt search-txt--big" | ||
| type="text" | ||
| data-qa="big" | ||
| placeholder="Try “Los Angeles“" | ||
| /> |
There was a problem hiding this comment.
Add empty lines between multiline sibling blocks of HTML to add some 'air' and simplify reading. There should be an empty line between the closing </div> on line 27 and the opening <div> on line 29.
| <input | ||
| class="search-txt search-txt--big" | ||
| type="text" | ||
| data-qa="big" | ||
| placeholder="Try “Los Angeles“" |
There was a problem hiding this comment.
Keep your attributes correctly formatted. The <input> tag starting on line 21 should have each attribute on a new line with proper indentation.
| <input | ||
| class="search-txt search-txt--small" | ||
| type="text" | ||
| data-qa="small" | ||
| placeholder="Try “Los Angeles“" |
There was a problem hiding this comment.
Keep your attributes correctly formatted. The <input> tag starting on line 30 should have each attribute on a new line with proper indentation.
| /* add styles here */ | ||
| body { | ||
| /* stylelint-disable-next-line font-family-name-quotes */ | ||
| 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. It's a good practice to specify a generic family as a fallback, such as sans-serif for Avenir in this case. For example: font-family: 'Avenir', Arial, sans-serif;
| input { | ||
| all: unset; | ||
| width: 100%; | ||
| height: 100%; | ||
| box-sizing: border-box; | ||
| border: 1px solid #e1e7ed; |
There was a problem hiding this comment.
Don't fix container size (if there is no such a requirement). In this case, setting both width: 100%; and height: 100%; on the input element might lead to unexpected behavior if the parent's size is not defined. It's usually better to control the size of an input with padding and possibly max-width or max-height.
| .search-box { | ||
| position: relative; | ||
| margin: 20px 0; | ||
| box-shadow: 0 1px 8px 0 #3d4e611a; | ||
| border-radius: 4px; | ||
| } |
There was a problem hiding this comment.
Be consistent with your margins (Add only top or bottom, but not both). Here, you've added a margin with margin: 20px 0; which is fine, but ensure that the rest of the elements follow the same convention of either top or bottom margin for consistency and to avoid margin collapse issues.
| @font-face { | ||
| /* stylelint-disable-next-line font-family-name-quotes */ | ||
| font-family: "Avenir" ; | ||
| src: url('./fonts/Avenir-Book.ttf') format('truetype'); | ||
| font-weight: 300; | ||
| } |
There was a problem hiding this comment.
When using the @font-face rule, it's important to define a set of font-weights and styles for better text rendering and to avoid faux styles in browsers that do not support the weight or style used in the CSS. Also, consider including a local source if the font is available on the user's system, which can save loading time.
