add solution #4489
add solution #4489
Conversation
There was a problem hiding this comment.
While you've done a great job overall, there are a few important details we need to address before we can move forward. Specifically, ensuring the form action attribute is a valid URL and that the method attribute is correctly specified are critical for the form to function properly. Additionally, taking care of the proper attribute formatting will help maintain the code's readability. Keep up the good work, and with these few adjustments, your code will be in top shape! 🛠️💼 Remember, these changes are important for the functionality and maintainability of your project.
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| <form action="post"> |
There was a problem hiding this comment.
The action attribute of the form element should be a valid URL. If you intend to use a POST method, make sure to specify it correctly with method="POST".
| /> | ||
| <form action="post"> | ||
| <input | ||
| class="input big-input" |
There was a problem hiding this comment.
Class names should represent the meaning of the content, not the styles or tag names. The class name 'big-input' seems to describe the style rather than the content or function of the input.
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| <input | ||
| class="input small-input" |
There was a problem hiding this comment.
Similar to the previous comment, the class name 'small-input' should be reconsidered to reflect the content or function rather than the style.
| } | ||
|
|
||
| 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 have a good set of fallback fonts here, but it's a good practice to also include a generic font family at the end, such as 'sans-serif' to ensure maximum compatibility.
| input { | ||
| display: block; | ||
| width: 100%; | ||
| margin-top: 20px; |
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 top margin, ensure that you're not also adding bottom margins elsewhere for the same element to avoid potential margin collapse.
| display: block; | ||
| width: 100%; | ||
| margin-top: 20px; | ||
| font-family: Avenir, Arial, Helvetica, sans-serif; |
There was a problem hiding this comment.
There's no need to set the font-family for input if it's the same as the body's font-family, unless you want to override a specific style from a more specific selector.
There was a problem hiding this comment.
Great job on your Pull Request! 🌟 All the feedback you've received are minor and more about consistency and best practices rather than critical functionality or security issues. Keep up the good work and always aim for improvement, paying attention to details like fallback fonts, consistent margins, and proper attribute formatting. These small touches will make your code even better! 💻✨
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <form method="post"> | ||
| <input | ||
| class="input big-input" | ||
| type="text" | ||
| data-qa="big" | ||
| 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 the number of attributes is more than 2, start each one, including the first, on a new line with 2-space indentation related to the tag.
| <input | ||
| class="input small-input" | ||
| type="text" | ||
| data-qa="small" | ||
| 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 the number of attributes is more than 2, start each one, including the first, on a new line with 2-space indentation related to the tag.
| input { | ||
| display: block; | ||
| width: 100%; | ||
| margin-top: 20px; |
There was a problem hiding this comment.
Be consistent with your margins (Add only top or bottom, but not both). In this case, you have added a top margin to the input. Ensure that you use margins consistently throughout your CSS to avoid potential margin collapse.
| 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've done this correctly here, but make sure to apply this practice throughout your project.
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 MistakesfromBEMlesson theory are checked.Code follows all the Code Style Rules ❗️