-
Notifications
You must be signed in to change notification settings - Fork 38
linoy project #26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
linoy project #26
Conversation
Tamir198
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey I left you some comments, to avoid repeat myself take every comment and implement the changes in every place in your code
| .attribution { font-size: 11px; text-align: center; } | ||
| .attribution a { color: hsl(228, 45%, 44%); } | ||
| </style> | ||
| <link rel="stylesheet" type="text/css" href="./style.css"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need the type?
| <div class="frontBox"> | ||
| <div class="allText"> | ||
|
|
||
| <h1 class="textPage" id="higlight">Stay updated!</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You dont really need both id and class
|
|
||
| <div class="mail"> | ||
| <label class="textPage" for="email" id="e-mail">Email address</label> | ||
| <input id="email" type="text" placeholder="email@example.com"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here why do you need id? you could achieve the same with class
| const errorMsg = document.getElementById("errorMsg"); | ||
|
|
||
| function isValidEmail(email) { | ||
| const regex = /^[a-zA-Z]{1,15}@[a-zA-Z]{1,15}\.com$/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex should be saved inside constant variable
| button.addEventListener("click", function() { | ||
| console.log("Button clicked!"); | ||
| const emailValue = emailInput.value.trim(); | ||
| console.log("Email" ,emailValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove your consoles, we dont want to push consoles.
Also I would add the ? symbol for null safety on the email
| @@ -0,0 +1,89 @@ | |||
| body { | |||
| background-color: hsl(235, 18%, 26%); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
colors should come from css variables
| } | ||
|
|
||
| @font-face { | ||
| font-family: 'myFont2'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need 2 font face?
| visibility: hidden; /* JS ישנה ל-visible */ | ||
| } | ||
|
|
||
| #e-mail { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be consistent with your naming, like-this or likeThis
| align-items: center; | ||
| justify-content: center; | ||
| border-radius: 5%; | ||
| height: 500px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be responsive in mobile?
| @@ -0,0 +1,15 @@ | |||
|
|
|||
| <html> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are you using this html page?
No description provided.