Develop #4557
Develop #4557
Conversation
There was a problem hiding this comment.
The issues with the @font-face rule and margin property need to be addressed as they could impact the styling and layout consistency. Please make these adjustments for a more robust implementation. Keep up the great work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/style.css
Outdated
| @font-face { | ||
| font-family: Avenir-Book; | ||
| src: url(./fonts/Avenir-Book.ttf); | ||
| font-family: Avenir-Heavy; | ||
| src: url(./fonts/Avenir-Heavy.ttf); | ||
| } |
There was a problem hiding this comment.
The @font-face rule is incorrectly structured. Each font-face declaration should be separate. You should not define multiple font-family and src properties within a single @font-face block.
| } | ||
|
|
||
| body { | ||
| margin: 0 8px; |
There was a problem hiding this comment.
The margin property should be consistent with the checklist recommendation to avoid potential margin collapse. Consider using only top or bottom margin, but not both.
There was a problem hiding this comment.
Great job on structuring the HTML file! 🎉 While there are minor issues with indentation, spacing, and font fallbacks, these are excellent learning opportunities. Please review the comments to enhance your code. Keep up the good work and continue improving!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| } | ||
|
|
||
| body { | ||
| margin: 0 8px; |
There was a problem hiding this comment.
Consider using only top or bottom margin to avoid potential margin collapse. The checklist suggests being consistent with your margins.
There was a problem hiding this comment.
I have couple comments here:
-
There is nothing stated in the task to set horizontal margins, I had to ask mentor to understand why tests were failing and he suggested putting "margin: 0 10px", but at 10px tests still were failing. I had to find the correct margin to comply the tests. I believe this point should be added to task description or tests should be updated
-
Why the bot says it's incorrect margins? I was suggested by mentor to set margins this way?
There was a problem hiding this comment.
@etojeDenys please check these questions
| border: 1px solid #e1e7ed; | ||
| font-weight: 300; | ||
| box-shadow: 0 1px 8px 0 #3d4e611a; | ||
| font-family: Avenir-Heavy, sans-serif; |
There was a problem hiding this comment.
It's a good practice to provide fallback fonts. Consider adding a generic font family like 'sans-serif' as a fallback for 'Avenir-Heavy'.
| } | ||
|
|
||
| .input::placeholder { | ||
| font-family: Avenir-Book, sans-serif; |
There was a problem hiding this comment.
Consider adding a generic font family like 'sans-serif' as a fallback for 'Avenir-Book' to ensure proper rendering if the custom font fails to load.
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 ❗️