first update #17
first update #17
Conversation
R0EYK
left a comment
R0EYK
left a comment
There was a problem hiding this comment.
Hi, left you some notes on your code, good job overall the code look pretty organized and nice to read.
Having some more practice with CSS with save you A LOT of code to set positions of stuff,
If you set your layout correctly it makes things MUCH easier but this will come slowly,
|
|
||
| emailInput.addEventListener("input", function () { | ||
| const email = emailInput.value; | ||
| const pattern = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; |
There was a problem hiding this comment.
Usually when you have such an ugly regex expression in your code, its important to leave a note and say what is it doing, otherwise if someone else would try to read your code it would be impossible to understand what is it doing unless you send it to some ai
There was a problem hiding this comment.
You could also call this variable emailPattern for clarity
| const pattern = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | ||
| if (email === "") { | ||
| emailInput.style.borderColor = "#ccc"; | ||
| emailInput.style.backgroundColor = "white"; |
There was a problem hiding this comment.
consider having a global constant for these things which might repeat multiple times
having something like `const WHITE = "white" , would make your code look nicer and more readable
| .container { | ||
| display: flex; | ||
| background: white; | ||
| width: 843px; |
There was a problem hiding this comment.
Usually its not recommended to give fixed dimensions to containers, it makes them unresponsive to changes in dimensions, consider controlling the dimensions of the container with vertical and horizontal paddings
| box-shadow: 0 10px 35px rgba(0,0,0,0.25); | ||
| } | ||
|
|
||
| #left-side { |
There was a problem hiding this comment.
See how in this id selector and also in #right-side you have given the same centering styles,
I would suggest you styling these with classes instead of ids then you could have a
.flex-center {
display: flex;
justify-content: center;
align-items: center;
}
this makes you use DO NOT REPEAT YOURSELF principle
| <label for="email">Email address</label> | ||
| <input type="email" id="email" placeholder="email@company.com"> | ||
|
|
||
| <button type="submit">Subscribe to monthly newsletter</button> |
There was a problem hiding this comment.
notice that you have a <button type="submit> element but its not wrapped in a <form> so it would behave like a normal button, I would suggest wrapping this in an actual form element which would give you some things which you will have to use JavaScript to get out of the box
| justify-content: center; | ||
| background: linear-gradient(135deg, #ff4664, #ff8f40); | ||
| width: 390px; | ||
| margin-top: 20px; |
There was a problem hiding this comment.
margin-top + margin-bottom = margin vertical
I know you know that but just a reminder :)
|
Hi Roey and Tamir. |
3 participants
No description provided.