Test1 #11
Test1 #11
Conversation
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Hey left you come commetns
generally - Do not use id so much, you can use class.
This is because class got less specificity than id, using id all over is kind of brute force solution
| <body> | ||
|
|
||
| <!-- Sign-up form start --> | ||
| <div class="main-card" id="signup-card"> |
There was a problem hiding this comment.
Why do you need both id and classes? you can use 2 classes
There was a problem hiding this comment.
there is no special reason, its just that I'm familiar with it
There was a problem hiding this comment.
It would be better to not always use id, only for spesific cases
| <!-- Success message end --> | ||
|
|
||
| <div class="attribution"> | ||
| <!-- <div class="attribution"> |
There was a problem hiding this comment.
Do not comment your code, delete it.
we can always check the history via git
| @@ -0,0 +1,91 @@ | |||
| // Getting elements | |||
There was a problem hiding this comment.
The first thing that came to my mind is "ok so this entire page was generated by ai"
You don't need all of those commends, this is a really bad practice
comments that explain what the code is doing are bad comments, or the code is bad and need clarification
Am i wrong to assume that this is ai code?
There was a problem hiding this comment.
not entirely I've used ai here for 2 things
- comments
- adding the fonts
| const showSuccessMessage = (email) => { | ||
| signupCard.classList.add('hidden'); | ||
| successCard.classList.remove('hidden'); | ||
| userEmailSpan.textContent = email; |
There was a problem hiding this comment.
You should check that your email if defined
| if (!emailDomain.includes(domain)) { | ||
| showError(); | ||
| return false; | ||
| } else { |
There was a problem hiding this comment.
In your if you have a return statement so you dont really need the else
Also you have 2 ifs that are doing exactly the same thing.
You can save the conditions inside a variables and combine those into single if with no else
| font-family: 'Roboto'; | ||
| src: url('./assets/fonts/Roboto-Bold.ttf') format('truetype'); | ||
| font-weight: bold; | ||
| font-style: normal; |
There was a problem hiding this comment.
This is the exact same code as above, remove it
| justify-content: center; | ||
| align-items: center; | ||
|
|
||
| background-color: hsl(234, 29%, 20%); |
There was a problem hiding this comment.
Colors should come from css variables and not inline like this
| .main-card{ | ||
| display: flex; | ||
| flex-direction: row; | ||
| background-color: hsl(0, 0%, 100%); |
There was a problem hiding this comment.
Same comment for all of the colors in this page
| } | ||
| .enabled-button{ | ||
| transition: background 0.3s ease; | ||
| /*adding gradient to button*/ |
There was a problem hiding this comment.
So this is ai generated? or you wrote it?
Any way remove it because comments like this are bad practice
| display: none; | ||
| } | ||
|
|
||
| /* Error States */ |
There was a problem hiding this comment.
My comment about comments in code apply to this entire page
2 participants
No description provided.