binyamin cohen #12
binyamin cohen #12
Conversation
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Hey I left you some comments
| <body> | ||
| <div class="successMessege"> | ||
| <img | ||
| src="assets\images\icon-success.svg" |
There was a problem hiding this comment.
src="assets/images/icon-success.svg" ?
Notice the / vs \
| <h1>Thanks for subscribing!</h1> | ||
| <p> | ||
| A confirmation email has been sent to | ||
| <b>ash@loremcompany.com.</b> Please open it and click the button inside |
There was a problem hiding this comment.
Do not style via tags but via css
There was a problem hiding this comment.
I can do ita with span class=email and do in css but its seems ogly in the html but if you want i can do it
There was a problem hiding this comment.
The reason we are not styling things in html is because we want to have separation of concerns, html should be used for putting elements and structore
And css should be the place where you style you structore
Just like in any other programming language we wont want to mix concepts into 1 place
| <p>Join 60,000+ product managers receiving monthly updates on:</p> | ||
|
|
||
| Stay updated! | ||
| <div class="allCheckMarks"> |
There was a problem hiding this comment.
Be consistent with your class names likeThis or like-this
| submitBtn.addEventListener("click", (e) => { | ||
| if (checkValidEmail(emailInput.value)) { | ||
| e.preventDefault(); | ||
| successMessege.classList.add("active"); |
There was a problem hiding this comment.
In here if you will add a return after the if you will not need the else
Also remove the console logs that you have, in production we never ship logging like this
| display: flex; | ||
| flex-direction: column; | ||
| height: fit-content; | ||
| width: fit-content; |
There was a problem hiding this comment.
Will it work if you remove both height and width?
if not why are you using those properties?
| padding: 10px; | ||
| cursor: pointer; | ||
| border-radius: 5px; | ||
| border: none; |
There was a problem hiding this comment.
Did you button had some border ? or why did you need to explicitly remove the border?
There was a problem hiding this comment.
ther is a litle gray border its boder me
| } | ||
| .error-message.active { | ||
| display: block; | ||
| color: red; |
There was a problem hiding this comment.
So color from variables comment above apply to this entire file
| align-items: flex-start; | ||
| background-color: white; | ||
| position: absolute; | ||
| z-index: 3; |
There was a problem hiding this comment.
You have 2 z-indexes in this file and they are both 3.
Why did you add those? and why 3 and not something lower?
| .successMessege p { | ||
| font-size: clamp(0.8rem, 2vw, 1.2rem); | ||
| } | ||
| .successMessege .success-icon { |
| display: flex; | ||
| flex-direction: column; | ||
| width: 100vw; | ||
| height: 110vh; |
There was a problem hiding this comment.
Why do you need this to be more tall than the view port?
There was a problem hiding this comment.
I think that means that your code is not responsive enough, for the task its fine
| A confirmation email has been sent to | ||
| <b>ash@loremcompany.com.</b> Please open it and click the button inside | ||
| to confirm your subscription. | ||
| <span class="companyEmail">ash@loremcompany.com.</span> Please open it |
| @@ -1,4 +1,9 @@ | |||
| @import url("https://fonts.googleapis.com/css2?family=Roboto:wght@400;700&display=swap"); | |||
| :root { | |||
| --error-color: #ff3333; | |||
| border-radius: 5px; | ||
| } | ||
| input:active::placeholder { | ||
| color: red; |
There was a problem hiding this comment.
Just this color bring form css variables as well
2 participants
No description provided.