Reut PR#3
Conversation
|
|
||
| <title>Frontend Mentor | Newsletter sign-up form with success message</title> | ||
| <link rel="stylesheet" href="styles.css"> | ||
| <script src="script.js" defer></script> |
There was a problem hiding this comment.
Why do you need the defer? whats the meaning of it?
There was a problem hiding this comment.
defer tells the browser to load the script in parallel but execute it only after the HTML has fully loaded. I understand it's not strictly required in this case, but in general I find it useful.
There was a problem hiding this comment.
Ye juat wanted to know that you know what you are doing and not blindly accepy ai changes
| </style> | ||
| </head> | ||
| <body> | ||
| <!-- Font --> |
There was a problem hiding this comment.
Remove the comments, they are not very helpfull
| <body> | ||
|
|
||
| Join 60,000+ product managers receiving monthly updates on: | ||
| <!-- מסך הטופס הראשי --> |
There was a problem hiding this comment.
Ok so no comments and even more no Hebrew comments ok?
| email@company.com | ||
| <div class="check_list"> | ||
| <div> | ||
| <img class="icon-list" src="assets/images/icon-list.svg" alt=""> |
| Product discovery and building what matters | ||
| </div> | ||
| <div> | ||
| <img class="icon-list" src="assets/images/icon-list.svg" alt=""> |
There was a problem hiding this comment.
In here you wrote icon-list but the other places are written like_this
Make sure to only pick one
| .button { | ||
| background-color: hsl(234, 29%, 20%); | ||
| border-radius: 8px; | ||
| border: none; |
There was a problem hiding this comment.
So border radius together with border none? is it working?
There was a problem hiding this comment.
No it actually doesn’t. That was my mistake:)
| @media (max-width: 768px) { | ||
|
|
||
| body { | ||
| display: block; |
There was a problem hiding this comment.
No, it wasn’t
But after checking again, I don’t think this line is actually necessary
There was a problem hiding this comment.
If you mean that the body is block by default, then yes
| box-shadow: none; | ||
| } | ||
|
|
||
| .right_container { |
There was a problem hiding this comment.
Can you think of more meaningful names for it ?
There was a problem hiding this comment.
I think image-container would be a better
| } | ||
|
|
||
| .image { | ||
| height: auto !important; |
There was a problem hiding this comment.
We never want to use important, it will override everything else
|
|
||
| /* success – mobile */ | ||
|
|
||
| @media (max-width: 768px) { |
There was a problem hiding this comment.
Isnt this the exact same media query like before? why do you need 2 ?
There was a problem hiding this comment.
One media query is for the main sign up screen and the other is for the success screen. I thought it would be cleaner to separate them into two
2 participants
No description provided.