Develop #4495

Krrampuss · 2024-09-27T15:43:20Z

mateacademy-ai-mentor

Hey there! 👋 It looks like there are a few areas in your code that could be improved. Specifically, the naming of classes such as '.big' and '.small' should be more descriptive to reflect the content or purpose rather than the style. Also, be mindful of using tag names for styling and consider adding more descriptive fallback fonts. Don't worry about the text shadow and container size for now, but do keep an eye on consistency with margins and indentation. Keep up the good work, and with a bit of tweaking, your code will be even better! 😊👍

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

mateacademy-ai-mentor · 2024-09-27T15:43:48Z

src/index.html

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


Class names should represent the meaning of the content, not the styles or tag names. The class name 'big form' on line 25 is not descriptive of the content. Consider using a class name that describes the role or content of the form or input element.

mateacademy-ai-mentor · 2024-09-27T15:43:48Z

src/index.html

+    <form
+      data-qa="small"
+      action="#"
+    >
+      <input
+        class="small form"
+        type="text"
+        placeholder="Try “Los Angeles“"
+      />


Similar to the previous comment, 'small form' as class names on line 37 are not descriptive. Use class names that reflect the content or purpose of the element.

src/style.css

mateacademy-ai-mentor · 2024-09-27T15:43:48Z

src/style.css

+.form {
+  font-family: Avenir, sans-serif;
+  display: block;
+  margin-top: 20px;


Be consistent with your margins. You've added a top margin here, so ensure you're not adding bottom margins elsewhere to the same element to avoid potential margin collapse.

mateacademy-ai-mentor · 2024-09-27T15:43:48Z

src/style.css

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


Don't fix container size (if there is no such a requirement). Here you've set a width of 100%, which is fine for responsiveness, but ensure that the content can dictate the size and that there's no overflow or accidental scrollbar.

mateacademy-ai-mentor · 2024-09-27T15:43:48Z

src/style.css

+}
+
+.big {
+  font-size: 16px;


Don't use tag names for styling (except 'html' and 'body'). It's better to use classes to style elements to avoid conflicts and increase reusability. Although '.big' is a class, the name could be confused with a styling utility rather than a meaningful class name.

mateacademy-ai-mentor · 2024-09-27T15:43:48Z

src/style.css

+}
+
+.small {
+  font-size: 14px;


The same comment as above for the '.small' class applies here. Consider using a more descriptive class name that represents the meaning of the content.

mateacademy-ai-mentor · 2024-09-27T15:43:48Z

src/style.css

+  background-size: contain;
+  text-shadow: drop-shadow(0 4px 4px rgba(0, 0, 0, 0.25));


Be cautious with text shadows as they can sometimes make text less readable. Ensure that this effect is actually enhancing the user experience.

mateacademy-ai-mentor

Your work is coming along well! 🌟 However, there are a few areas that need attention before we can approve the Pull Request. Please consider the following:

Ensure content dictates container size and avoid fixed heights to prevent overflow.
Use consistent margins to avoid margin collapse.
Include a fallback font-family for better cross-browser compatibility.
Use hyphens or underscores in class names instead of spaces for better readability.
Reflect the purpose of elements in class names rather than their style or HTML tags.
Properly format and indent your HTML attributes for better code readability.

These changes will help maintain the functionality and improve the quality of your code. Keep up the good work, and once these adjustments are made, your Pull Request should be good to go! 👍

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

src/index.html

src/style.css

mateacademy-ai-mentor · 2024-09-27T15:54:42Z

src/style.css

+.form {
+  font-family: Avenir, sans-serif;
+  display: block;
+  margin-top: 20px;


It's recommended to use margins consistently, either top or bottom, but not both, to avoid potential margin collapse issues.

src/style.css

mateacademy-ai-mentor

Great job on the progress! 👍 Remember to avoid setting fixed sizes on containers unless required, maintain consistent margins, use fallback font families, and ensure proper attribute formatting. Your class naming should be meaningful, reflecting content rather than styles. Keep these points in mind for future work! Happy coding! 😊

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

mateacademy-ai-mentor · 2024-09-27T16:10:10Z

src/index.html

-      data-qa="keypress"
-      placeholder="Try “Los Angeles“"
-    />
+  <body class="body">


The class name 'body' does not represent the meaning of the content. Class names should reflect the purpose or identity of the content, not the tag name or styles being applied. Consider using a more meaningful class name that describes the role or type of content within the element.

mateacademy-ai-mentor · 2024-09-27T16:10:10Z

src/index.html

+    <form
+      data-qa="big"
+      action="#"
+    >


Keep attributes correctly formatted: Since there are more than two attributes, each one should start on a new line with 2-space indentation relative to the tag, and the closing bracket should be on the same level as the opening tag.

mateacademy-ai-mentor · 2024-09-27T16:10:11Z

src/index.html

+      <input
+        class="search_input_big
+        form"
+        type="text"
+        data-qa="keypress"
+        placeholder="Try “Los Angeles“"


The class attribute value is not properly closed with a quotation mark. This might cause unexpected behavior or errors in your HTML.

mateacademy-ai-mentor · 2024-09-27T16:10:11Z

src/index.html

+    <form
+      data-qa="small"
+      action="#"
+    >


Keep attributes correctly formatted: Since there are more than two attributes, each one should start on a new line with 2-space indentation relative to the tag, and the closing bracket should be on the same level as the opening tag.

mateacademy-ai-mentor · 2024-09-27T16:10:11Z

src/index.html

+        class="search_input_small
+         form"
+        type="text"
+        placeholder="Try “Los Angeles“"


The class attribute value is not properly closed with a quotation mark, and there is an inconsistent indentation in the class attribute value. Ensure that the value is properly enclosed with quotation marks and follow the 2-space indentation rule.

mateacademy-ai-mentor · 2024-09-27T16:10:11Z

src/style.css

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


Remember to use fallback fonts - alternative font-family in case the main one doesn't work. While you have provided Arial and Helvetica as fallbacks, it's a good practice to always include a generic family name at the end, such as 'sans-serif'.

mateacademy-ai-mentor · 2024-09-27T16:10:11Z

src/style.css

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


As mentioned before, consider adding a generic family name as the last fallback for the font-family property.

mateacademy-ai-mentor · 2024-09-27T16:10:11Z

src/style.css

+.form {
+  font-family: Avenir, Arial, Helvetica, sans-serif;
+  display: block;
+  margin-top: 20px;


Be consistent with your margins. You've added a margin-top here, but ensure that you're not using both margin-top and margin-bottom for other elements to avoid potential margin collapse.

mateacademy-ai-mentor · 2024-09-27T16:10:11Z

src/style.css

+.search_input_big{
+  font-size: 16px;
+  background: url(images/Search.svg) no-repeat;
+  background-size: 19px;
+  box-sizing: border-box;
+  background-position: 26px center;
+  padding-left: 62px;
+  height: 70px;
+  background-color: #ffff;


Don't fix container size if there is no such requirement. Here you have set a fixed height for the search input. Make sure that the content can dictate the size to avoid overflow or accidental scroll bar. If the fixed height is not a specific requirement, consider using padding and line-height to control the size.

mateacademy-ai-mentor · 2024-09-27T16:10:11Z

src/style.css

+.search_input_small{
+  font-size: 14px;
+  box-shadow: 0 1px 8px 0 #3d4e611a;
+  background: url(images/Search.svg) no-repeat 13px center/11px 11px;
+  height: 42px;
+  padding-left: 33px;
+}


Similar to the previous comment, avoid fixing the size of containers like input fields unless explicitly required.

BudnikOleksii

Good work 👍

Krrampuss added 2 commits September 25, 2024 17:29

npm i fix

ccf14b1

add task solution

3dc6a9c

mateacademy-ai-mentor suggested changes Sep 27, 2024

View reviewed changes

add

db6c7e0

Krrampuss requested a review from mateacademy-ai-mentor September 27, 2024 15:54

mateacademy-ai-mentor suggested changes Sep 27, 2024

View reviewed changes

Krrampuss added 3 commits September 27, 2024 19:02

add2

457b8b7

add3

1ccc226

add4

05557ec

Krrampuss requested a review from mateacademy-ai-mentor September 27, 2024 16:09

mateacademy-ai-mentor approved these changes Sep 27, 2024

View reviewed changes

BudnikOleksii approved these changes Sep 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Develop #4495

Develop #4495

Krrampuss commented Sep 27, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

mateacademy-ai-mentor Sep 27, 2024

BudnikOleksii left a comment

		background-size: contain;
		text-shadow: drop-shadow(0 4px 4px rgba(0, 0, 0, 0.25));

Develop #4495

Are you sure you want to change the base?

Develop #4495

Conversation

Krrampuss commented Sep 27, 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

mateacademy-ai-mentor left a 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

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

BudnikOleksii left a comment

Choose a reason for hiding this comment