finish-design #6
Conversation
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Some general notes:
- its bad practice to comment out code, remove it if not needed
- colors should be saved inside css variables and not hardcoded
Other than that good work, I left you some comments
| <h1>Stay updated!</h1> | ||
| <p>Join 60,000+ product managers receiving monthly <br> updates on:</p> | ||
| <div class="line"> | ||
| <img class="ok-img" src="assets/images/icon-success.svg" alt=""> |
There was a problem hiding this comment.
For accessibility its you should fill out the alt attribute
| <h1>Stay updated!</h1> | ||
| <p>Join 60,000+ product managers receiving monthly <br> updates on:</p> | ||
| <div class="line"> | ||
| <img class="ok-img" src="assets/images/icon-success.svg" alt=""> |
There was a problem hiding this comment.
For accessibility its you should fill out the alt attribute
| And much more! | ||
| </div> | ||
| <form> | ||
| <label class="Email-label" for="email-textbox">Email address</label> |
There was a problem hiding this comment.
You want to be consistent with your other code so I would write this as email-label without the capital e
| And much more! | ||
| </div> | ||
| <form> | ||
| <label class="Email-label" for="email-textbox">Email address</label> |
There was a problem hiding this comment.
You want to be consistent with your other code so I would write this as email-label without the capital e
| <!-- Success message start --> | ||
|
|
||
| Thanks for subscribing! | ||
| <!-- Thanks for subscribing! |
There was a problem hiding this comment.
Commenting code is bad practice, remove it.
we can always use git to check what was here
| @@ -0,0 +1,136 @@ | |||
| * { | |||
| box-sizing: border-box; | |||
| padding: 0; | ||
| } | ||
|
|
||
| .right-side > p { |
There was a problem hiding this comment.
This will select every direct p to the .right-side
If you need something more specific give class to your p and use it in here
| align-self: center; | ||
| } | ||
|
|
||
| .right-side form { |
There was a problem hiding this comment.
If you will have more than 1 form inside right-side this css will affect him as well and we will not always want to do so
So the solution should be the same as in the comment above - add class name for your form
| } | ||
|
|
||
|
|
||
| @media (min-width: 768px) { |
There was a problem hiding this comment.
Nice work in supporting mobile as well, btw this is supported as well :
@media (width <= 1250px) {For more info check the docs
2 participants
No description provided.