ad solution #4533

t-namestnykova · 2024-10-11T11:10:21Z

DEMO LINK
TEST REPORT LINK
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 ❗️

mateacademy-ai-mentor

Great job so far! 😊 However, there are a few areas that need improvements. Please consider the feedback on your CSS file, such as avoiding the use of the * selector for all elements, keeping font family consistent, and making your class names more meaningful. Once you've made these changes, your code will be even better! Keep going! 💪

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

mateacademy-ai-mentor · 2024-10-11T11:10:50Z

src/index.html

+ <form
+ class="search-bar search-bar-top"
+ data-qa="big"
+ >
+ <input
+ class="search-bar-input search-bar-input--top"
+ type="text"
+ data-qa="keypress"
+ placeholder="Try “Los Angeles“"
+ />


You have two forms here that seem to be doing the same thing. Unless they have different purposes, you might want to consider combining them into one form to avoid repetition. This will make your code more DRY (Don't Repeat Yourself), which is a key principle in software development.

mateacademy-ai-mentor · 2024-10-11T11:10:50Z

src/index.html

+ <form
+ class="search-bar search-bar-bottom"
+ data-qa="small"
+ >
+ <input
+ class="search-bar-input search-bar-input--bottom"
+ type="text"
+ data-qa="hover"
+ placeholder="Try “Los Angeles“"
+ />


Same as above, consider combining these forms if they serve the same purpose.

mateacademy-ai-mentor · 2024-10-11T11:10:50Z

src/index.html

+ class="search-bar search-bar-top"
+ data-qa="big"
+ >
+ <input


The class name 'search-bar-input--top' and 'search-bar-input--bottom' do not represent the meaning of the content. Consider renaming these classes to something more meaningful. For example, if the class is intended to style the input differently based on its position, a name like 'search-bar-input-upper' and 'search-bar-input-lower' might be more appropriate.

mateacademy-ai-mentor · 2024-10-11T11:10:50Z

src/index.html

+ class="search-bar search-bar-bottom"
+ data-qa="small"
+ >
+ <input


Same as above, consider renaming 'search-bar-input--bottom' to something more meaningful.

mateacademy-ai-mentor · 2024-10-11T11:10:50Z

src/style.css

+ font-style: normal;
+ color: #3d4e61;
+ box-sizing: border-box;
+}


You're using the * selector to set the font family, font weight, font style, color, and box-sizing for all elements. This is not recommended as it can impact performance. It's better to set these properties on specific elements that require them. (Refer to the CSS checklist)

mateacademy-ai-mentor · 2024-10-11T11:10:50Z

src/style.css

+}
+
+* {
+ font-family: Avenir, Arial, Helvetica, sans-serif;


You've set the font family to Avenir, Arial, Helvetica, sans-serif. This is good, but remember to use fallback fonts - alternative font-family in case the main one doesn't work. (Refer to the CSS checklist)

mateacademy-ai-mentor · 2024-10-11T11:10:50Z

src/style.css

+}
+
+.search-bar {
+ margin-top: 20px;


You've added a margin-top to the .search-bar class. It's better to be consistent with your margins. Add only top or bottom, but not both, to avoid potential margin collapse. (Refer to the CSS checklist)

mateacademy-ai-mentor · 2024-10-11T11:10:50Z

src/style.css

+ border: 1px solid #e1e7ed;
+ border-radius: 4px;
+ box-shadow: 0 1px 8px 0 #3d4e611a;
+}


You've set the width and height of the .search-bar-input class to 100%. This can cause issues if the parent container's size is not fixed. It's generally better not to fix container size, and let the content size dictate it. This helps to avoid overflow or accidental scroll bar. (Refer to the CSS checklist)

mateacademy-ai-mentor · 2024-10-11T11:10:51Z

src/style.css

+ box-shadow: rgba(0, 0, 0, 0.25);
+ font-family: Avenir-Heavy, sans-serif;
+ outline: none;
+}


In the .search-bar-input:focus class, you've changed the font family. This can lead to a sudden change in the layout when the input is focused, which may not be a good user experience. It's better to keep the font family consistent across different states of an element.

mateacademy-ai-mentor

Great job on your HTML and CSS! 😊 However, there are a few improvements to be made in your CSS code. Please ensure the correct use of font-family and consider not fixing container sizes unless necessary. Also, remember to include a fallback font. Keep up the good work and make these minor changes to enhance your code quality! 👍

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

mateacademy-ai-mentor · 2024-10-11T16:08:05Z

src/style.css

+@font-face {
+ font-family: Avenir-Heavy;


It seems like you've declared a new font-family 'Avenir-Heavy'. Ideally, you should only declare the base font family once (i.e., 'Avenir') and then use the 'font-weight' property to control the boldness of the font. The browser will then choose the closest available font weight. This is important for the consistency and maintainability of your CSS code.

mateacademy-ai-mentor · 2024-10-11T16:08:05Z

src/style.css

+ border-radius: 4px;
+ box-shadow: 0 1px 8px 0 #3d4e611a;
+
+ font-family: Avenir, sans-serif;


You've declared the 'Avenir' font here, but there's no fallback font. It's a good practice to always include a fallback font, in case the custom font doesn't load. For example, you can use a generic font family like 'sans-serif' as a fallback.

mateacademy-ai-mentor · 2024-10-11T16:08:05Z

src/style.css

+}
+
+.search-bar {
+ margin-top: 20px;


It's recommended to be consistent with your margins. Here you've added a top margin, but in a real-world scenario, it's better to stick to just one direction (either top or bottom) to avoid potential margin collapse problems.

mateacademy-ai-mentor · 2024-10-11T16:08:05Z

src/style.css

+
+.search-bar-top {
+ height: 70px;
+ margin-top: 20px;


Be careful with fixing container size. In this case, you've fixed the height for '.search-bar-top'. Unless it's a specific requirement, it's better to let the content dictate the size of the container to avoid overflow or accidental scroll bar.

mateacademy-ai-mentor · 2024-10-11T16:08:05Z

src/style.css

+}
+
+.search-bar-bottom {
+ height: 42px;


The same comment about fixing container size applies here for '.search-bar-bottom'. It's better to let the content size dictate the container size.

mateacademy-ai-mentor · 2024-10-11T16:08:05Z

src/style.css

+ border-radius: 4px;
+ box-shadow: 0 1px 8px 0 #3d4e611a;
+
+ font-family: Avenir, sans-serif;


You've used the 'Avenir' font here, but it seems you've declared a new font-family 'Avenir-Heavy' at the beginning of your CSS. Remember to use the correct font-family name when applying it to your elements.

mateacademy-ai-mentor

Great job on the HTML and CSS! The code is clean and well-structured. However, there are a few things to note. Firstly, always provide a fallback font-family in case your chosen font fails to load. Secondly, use semantic names for your font-families and control the weight via the 'font-weight' property. Keep up the good work and continue to improve your coding skills! 👍

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

mateacademy-ai-mentor · 2024-10-11T17:08:57Z

src/style.css

+}
+
+@font-face {
+ font-family: Avenir-Heavy;


The font-family name 'Avenir-Heavy' is not semantic. It would be better to use 'Avenir' as the font-family for all related fonts, and control the weight via the 'font-weight' property. This way, you can easily switch between font weights without having to change the font-family.

mateacademy-ai-mentor · 2024-10-11T17:08:57Z

src/style.css

+ border-radius: 4px;
+ box-shadow: 0 1px 8px 0 #3d4e611a;
+
+ font-family: Avenir, sans-serif;


You have specified 'Avenir' as the font-family, but you haven't provided a fallback font. If the 'Avenir' font fails to load for any reason, the browser will use the default font, which may not fit your design. It's a good practice to specify a fallback font that is widely available across different systems. For example: font-family: Avenir, Arial, sans-serif;

mateacademy-ai-mentor · 2024-10-11T17:08:57Z

src/style.css

+.search-bar__input:focus {
+ box-shadow: 0 1px 8px 0 rgba(0, 0, 0, 0.25);
+ outline: none;
+ font-family: Avenir-Heavy, sans-serif;


Same as before, remember to provide a fallback font-family.

ad solution

efa03e7

mateacademy-ai-mentor suggested changes Oct 11, 2024

View reviewed changes

t-namestnykova added 4 commits October 11, 2024 15:00

solution

8ce756a

add changes to readme.md

3b2239f

solution

107f12c

add solution

d44f3ff

t-namestnykova requested a review from mateacademy-ai-mentor October 11, 2024 16:07

mateacademy-ai-mentor suggested changes Oct 11, 2024

View reviewed changes

solution with changes

141c968

t-namestnykova requested a review from mateacademy-ai-mentor October 11, 2024 17:08

mateacademy-ai-mentor approved these changes Oct 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ad solution #4533

ad solution #4533

t-namestnykova commented Oct 11, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

mateacademy-ai-mentor Oct 11, 2024

ad solution #4533

Are you sure you want to change the base?

ad solution #4533

Conversation

t-namestnykova commented Oct 11, 2024

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment