liron pr #20
liron pr #20
Conversation
R0EYK
left a comment
R0EYK
left a comment
There was a problem hiding this comment.
Hi , I have left you some comments on your code.
The code looks REALLY nice.
| document.querySelector(".subscribe-success p b").textContent = email; | ||
| subscribeSuccessWindow.classList.add("active"); | ||
| } | ||
| } No newline at end of file |
newsletter-sign-up-with-success-message-main/subscribe-page-design.css
Outdated
Show resolved
Hide resolved
newsletter-sign-up-with-success-message-main/subscribe-page-design.css
Outdated
Show resolved
Hide resolved
| } | ||
| .subscribe-success.active { | ||
| max-width: 340px; | ||
| height: 350px; |
There was a problem hiding this comment.
In stuff like border-radius, paddings , margings, try working with different measurments than percentages, rem is the most recommended and used one
There was a problem hiding this comment.
what about width and height? and the breakpoints in the media queries?
| font-family: "Roboto", sans-serif; | ||
| } | ||
|
|
||
| @media(min-width: 528px) { |
ShirYahav
left a comment
ShirYahav
left a comment
There was a problem hiding this comment.
Needs Improvement:
- Improve HTML semantics and structure.
- Review where to declare const variables.
- Prefer responsive units over px.
|
|
||
| <!-- Sign-up form end --> | ||
| <article class="subscribe-form"> | ||
| <picture> |
There was a problem hiding this comment.
but I have read the the best practice for having a picture's size dynamically change dependent of the screen's size is using
<picture> <source media="" srcset=""> <source media="" srcset=""> <img src="" alt=" > </picture>
There was a problem hiding this comment.
The structure and usage of elements here could be improved. Consider pasting your code into an AI to get suggestions for a cleaner, more semantic structure
| emailInput.classList.add("invalid"); | ||
| document.getElementById(".inalid-email-label").classList.add("invalid"); | ||
| } else { | ||
| const form = event.target; |
There was a problem hiding this comment.
you want to define the const variables oustside of the function - read why
There was a problem hiding this comment.
but in this case those variables depend of values that exist only inside the function's scope
| font-family: "Roboto", sans-serif; | ||
| } | ||
|
|
||
| @media(min-width: 528px) { |
There was a problem hiding this comment.
Choose either mobile-first or desktop-first as your default approach, no need to specify both
(paste comment to ai it will explain it)
| @media(min-width: 528px) { | ||
| .subscribe-form { | ||
| max-height: 400px; | ||
| max-width: 800px; |
There was a problem hiding this comment.
Try to avoid using px, there are more responsive units (like rem, em, etc...) read about it
3 participants
No description provided.