Feature/after comments #30
Conversation
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Hey i left you some cooments, they are relevant to every place in the code.
I just didnt want to repeat myself a lot
| @@ -0,0 +1,28 @@ | |||
| const emailPattern = document.getElementById("email"); | |||
| const CCC = "#ccc" | |||
There was a problem hiding this comment.
There should not be colors in here, they should sit inside css variable object, also ccc is not a good name, the name should indicate the color and not the hexa value
| @@ -0,0 +1,28 @@ | |||
| const emailPattern = document.getElementById("email"); | |||
There was a problem hiding this comment.
This is not the patterns but the element
| const email = emailPattern.value; | ||
| const pattern = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | ||
| if (email === "") { | ||
| emailPattern.style.borderColor = "CCC"; |
There was a problem hiding this comment.
Never style via js, only from css and dynamic classes
|
|
||
|
|
||
| body { | ||
| background-color: hsl(234, 29%, 20%); |
There was a problem hiding this comment.
bring the colors from css variables
|
|
||
| .container { | ||
| display: flex; | ||
| background: white; |
There was a problem hiding this comment.
Same here and for all of the similar places
| background: white; | ||
| width: 45%; | ||
| height: 55%; | ||
| border-radius: 25px; |
There was a problem hiding this comment.
Try not to mix different units, go with 1
| justify-content: center; | ||
| padding: 50px; | ||
| width: 43%; | ||
| height: auto; |
There was a problem hiding this comment.
Do you really need this? will it have any effect?
|
|
||
| } | ||
|
|
||
| #left-side h1 { |
There was a problem hiding this comment.
Can you think of a better name than left side? what if this will not be on the left side in the future?
| </p> | ||
|
|
||
| <ul> | ||
| <li><img src="assets/images/icon-list.svg" alt="">Product discovery and building what matters</li> |
There was a problem hiding this comment.
Empty alt means nothing in terms of accessibility
2 participants
No description provided.