finished #29
finished #29
Conversation
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Hey i left you some comments, please make sure to take every comment and apply it to all of the relevant places in the code
| <link rel="icon" type="image/png" sizes="32x32" href="./assets/images/favicon-32x32.png"> | ||
|
|
||
|
|
||
| <script src="./script.js" defer></script> |
There was a problem hiding this comment.
Why do you need the defer in here?
| <!-- Sign-up form start --> | ||
|
|
||
| Stay updated! | ||
| <div class="card" id="first-card"> |
There was a problem hiding this comment.
Why do you need both is and class?
|
|
||
| Join 60,000+ product managers receiving monthly updates on: | ||
| <p><img class="icon" src="./assets/images/icon-list.svg"/> Product discovery and building what matters</p> | ||
| <p><img class="icon" src="./assets/images/icon-list.svg"/> Measuring to ensure updates are a success</p> |
There was a problem hiding this comment.
inside p tag we only place text not images
|
|
||
| Subscribe to monthly newsletter | ||
| <div id="success-card" class="card"> | ||
| <!-- Sign-up form end --> |
There was a problem hiding this comment.
Remove your comments, they are not needed
| <!-- Success message start --> | ||
|
|
||
| <!-- Success message start --> | ||
| <img class="v-icon" src="./assets/images/icon-success.svg"/> |
There was a problem hiding this comment.
if we will change this icon to not v you will need to change the name.
Can you think of a better name for this?
| // txt.addEventListener("input", () => { | ||
| //ok | ||
| if (txt.value.includes("@")) { | ||
| document.getElementById("errormsg").style.display = "none"; |
There was a problem hiding this comment.
We dont want to style via js, do it with dynamic classes and css
| document.getElementById("subscribe").addEventListener("click", () => { | ||
|
|
||
| if (checkEmailValidity()){ | ||
| let email = document.querySelector(".mail").value; |
There was a problem hiding this comment.
You are creating the variable again and again on every event trigger, create it on top of the file
| border-radius: 15px; | ||
| width: 45vw; | ||
| height: 50vh; | ||
| left: 25vw; |
There was a problem hiding this comment.
Do not mix so many different units, pick 1
| @@ -0,0 +1,228 @@ | |||
| html { | |||
| background-color: rgb(49, 50, 62); | |||
There was a problem hiding this comment.
Colors should come from the css variables
| color: red; | ||
| } | ||
|
|
||
| span p { |
There was a problem hiding this comment.
this will affect all of the p thats inside span, use class to be more spesific
3 participants
No description provided.