Skip to content

Conversation

@4p4hjn5bsq-sudo
Copy link

No description provided.

Copy link
Member

@Tamir198 Tamir198 left a 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">
Copy link
Member

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>
Copy link
Member

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">
Copy link
Member

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$/;
Copy link
Member

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);
Copy link
Member

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%);
Copy link
Member

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';
Copy link
Member

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 {
Copy link
Member

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;
Copy link
Member

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>
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants