Feature/index structure#4
Conversation
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Hey I left you some change request, generally good work
| @@ -0,0 +1,75 @@ | |||
| /* | |||
| - Mobile: 375px | |||
| - Desktop: 1440px | |||
There was a problem hiding this comment.
When you are making a pr do not keep those comments, looks like ai generated comment?
| */ | ||
|
|
||
| :root { | ||
| font-family: 'Roboto', sans-serif; |
|
|
||
| body { | ||
| background-color: var(--originalBlue800); | ||
| width: 98vw; |
There was a problem hiding this comment.
any reason not to use 100vw? why would we want the body not to be 100% in width?
| justify-content: center; | ||
| } | ||
|
|
||
| #form-text { |
There was a problem hiding this comment.
In here you are using id instead of classes, any reason for this?
because id got higher specificity than class ?
|
|
||
| #desktop-image img { | ||
| max-width: 100%; | ||
| /* max-height: 100%; */ |
There was a problem hiding this comment.
Please do not push commended code, remove it, we got git for seeing history
|
|
||
| <!-- Sign-up form start --> | ||
| <div class="form-container"> | ||
| <div class="form-column" id="form-text"> |
There was a problem hiding this comment.
So in here you are using both id and class, you dont really need both of them, you can remain with only class
|
|
||
| Subscribe to monthly newsletter | ||
| <div class="form-column"> | ||
| <div id="desktop-image"> |
There was a problem hiding this comment.
This is not really the desktop image, this is the container so I would change the name desktop-image-container
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Hey I left you some comments in here for you to change, good work
| </div> | ||
|
|
||
| Subscribe to monthly newsletter | ||
| <div class="form-column"> |
There was a problem hiding this comment.
HTML is like a book for developers, and we all know that form label means that we have a form inside so you could use form instead of your div
So its more readable
| Subscribe to monthly newsletter | ||
| <div class="form-column"> | ||
| <div id="desktop-image"> | ||
| <img src="./assets/images/illustration-sign-up-desktop.svg" alt="Sign up illustration"> |
There was a problem hiding this comment.
Good work on the alt attribute
2 participants
No description provided.