Designed Newsletter sign-up form#5
Conversation
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Here are some general notes:
- Please use css variables for colors and not write them hardcoded
|
|
||
| <!-- Feel free to remove these styles or customise in your own stylesheet 👍 --> | ||
| <style> | ||
| <!-- <style> |
There was a problem hiding this comment.
To track changes you have git, we dont need to leave commented code because this is a bad practice
| <body> | ||
|
|
||
| <!-- Sign-up form start --> | ||
| <div class="form"> |
There was a problem hiding this comment.
In terms of semantic you could also use the
form
tag
Why? HTML is like a book for developers, and we all know that form label means that we have a form
So its more readable
|
|
||
| <!-- Sign-up form start --> | ||
| <div class="form"> | ||
| <div class="left"> |
There was a problem hiding this comment.
IMO - I think that we can find a better name for this, and this is because if we will have a lot of html that should go to the left of some parent then it will be hard to distinguish between all of the other "left" parts
| <div class="left"> | ||
| <div class="text"> | ||
| <p class="header">Stay Updated!</p> | ||
| <p class="small">Join 60,000+ product managers receiving monthly<br>updates on:</p> |
There was a problem hiding this comment.
Its not recommended to use br tag, its a bad practice.
Please space your text with css
| <div class="checkbox"> | ||
| <div class="row"> | ||
| <img src="./assets/images/icon-list.svg" id="icon"> | ||
| <p class="small" id="icons">Product discovery and building what matters</p> |
There was a problem hiding this comment.
Why do you need both class and id? you can have multiple classes assigned into one element and this way you will not need to use id
|
|
||
| Stay updated! | ||
| </div> | ||
| <!-- Stay updated! |
There was a problem hiding this comment.
Its a bad practice to leave commented code, we have git to trace changes for it please remove this
| <!-- Success message start --> | ||
|
|
||
| Thanks for subscribing! | ||
| <!-- Thanks for subscribing! |
There was a problem hiding this comment.
I dont want to write you tons of times, so generally take the above comment about the commented out code and implement it all over this file
| align-items: center; | ||
| width: 100%; | ||
| height: 100%; | ||
| font-family: system-ui, -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen, Ubuntu, Cantarell, 'Open Sans', 'Helvetica Neue', sans-serif; |
There was a problem hiding this comment.
Hey why do you need all of those fallbacks? didnt the task already had font inside a folder?
| margin-bottom: 28px; | ||
| margin-top:40px; | ||
| } | ||
| #icon{ |
There was a problem hiding this comment.
Why do you need to use id? can you do the same with class?
| width: 365px; | ||
| height:36px; | ||
| } | ||
| #placeholder{ |
There was a problem hiding this comment.
Same comment in here about the class usage
2 participants
No description provided.