Skip to content

Reut PR #3

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.

Sign up for GitHub

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

Open
reutmaduel114-collab wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Reut PR #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ed31b1
test
reutmaduel114-collab Nov 16, 2025
2d497e1
upload
reutmaduel114-collab Nov 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 63 additions & 31 deletions newsletter-sign-up-with-success-message-main/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,52 +1,84 @@
<!DOCTYPE html>
<html lang="en">

<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0"> <!-- displays site properly based on user's device -->
<meta name="viewport" content="width=device-width, initial-scale=1.0">

<link rel="icon" type="image/png" sizes="32x32" href="./assets/images/favicon-32x32.png">

<title>Frontend Mentor | Newsletter sign-up form with success message</title>
<link rel="stylesheet" href="styles.css">
<script src="script.js" defer></script>

@Tamir198 Tamir198 Nov 30, 2025

Copy link
Copy Markdown

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 defer? whats the meaning of it?

@reutmaduel114-collab reutmaduel114-collab Dec 1, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defer tells the browser to load the script in parallel but execute it only after the HTML has fully loaded. I understand it's not strictly required in this case, but in general I find it useful.

@Tamir198 Tamir198 Dec 1, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ye juat wanted to know that you know what you are doing and not blindly accepy ai changes


<!-- Feel free to remove these styles or customise in your own stylesheet 👍 -->
<style>
.attribution { font-size: 11px; text-align: center; }
.attribution a { color: hsl(228, 45%, 44%); }
</style>
</head>
<body>
<!-- Font -->

@Tamir198 Tamir198 Nov 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the comments, they are not very helpfull

<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<link href="https://fonts.googleapis.com/css2?family=Roboto:wght@400;700&display=swap" rel="stylesheet">

<!-- Sign-up form start -->
<title>Frontend Mentor | Newsletter sign-up form with success message</title>
</head>

Stay updated!
<body>

Join 60,000+ product managers receiving monthly updates on:
<!-- מסך הטופס הראשי -->

@Tamir198 Tamir198 Nov 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so no comments and even more no Hebrew comments ok?

<div class="main_container">
<div class="left_container">

Product discovery and building what matters
Measuring to ensure updates are a success
And much more!
<header class="header">Stay updated!</header>
<div>Join 60,000+ product managers receiving monthly updates on:</div>

Email address
email@company.com
<div class="check_list">
<div>
<img class="icon-list" src="assets/images/icon-list.svg" alt="">

@Tamir198 Tamir198 Nov 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty alt does not do anything

Product discovery and building what matters
</div>
<div>
<img class="icon-list" src="assets/images/icon-list.svg" alt="">

@Tamir198 Tamir198 Nov 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In here you wrote icon-list but the other places are written like_this
Make sure to only pick one

Measuring to ensure updates are a success
</div>
<div>
<img class="icon-list" src="assets/images/icon-list.svg" alt="">
And much more!
</div>
</div>

Subscribe to monthly newsletter
<div class="email_container">
<div class="email_row">
<label class="email_label">Email address</label>
<span class="email_error">Valid email required</span>
</div>
<input class="email_box" type="email" id="email" placeholder="email@company.com">

@Tamir198 Tamir198 Nov 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need both class and id?

</div>

<!-- Sign-up form end -->
<button class="button subscribe-btn">Subscribe to monthly newsletter</button>
</div>

<!-- Success message start -->
<div class="right_container">
<!-- for desktop -->
<img class="image image-desktop" src="./assets/images/illustration-sign-up-desktop.svg" alt="">
<!-- for mobile -->
<img class="image image-mobile" src="./assets/images/illustration-sign-up-mobile.svg" alt="">
</div>
</div>

Thanks for subscribing!
<!-- מסך הצלחה -->
<div class="success_container">
<div class="success_content">
<div class="success_icon">
<img src="./assets/images/icon-success.svg" alt="Success">
</div>

A confirmation email has been sent to ash@loremcompany.com.
Please open it and click the button inside to confirm your subscription.
<h1 class="success_title">Thanks for subscribing!</h1>

Dismiss message
<p class="success_text">
A confirmation email has been sent to
<span class="success-email">ash@loremcompany.com</span>.
Please open it and click the button inside to confirm your subscription.
</p>

<!-- Success message end -->

<div class="attribution">
Challenge by <a href="https://www.frontendmentor.io?ref=challenge" target="_blank">Frontend Mentor</a>.
Coded by <a href="#">Your Name Here</a>.
<button class="button dismiss-btn">Dismiss message</button>

@Tamir198 Tamir198 Nov 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give this button a more specific name?

</div>
</div>

</body>
</html>

</html>
55 changes: 55 additions & 0 deletions newsletter-sign-up-with-success-message-main/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
const emailInput = document.getElementById("email");
const errorText = document.querySelector(".email_error");
const subscribeBtn = document.querySelector(".subscribe-btn");

const mainContainer = document.querySelector(".main_container");
const successContainer = document.querySelector(".success_container");
const successEmailSpan = document.querySelector(".success-email");
const dismissBtn = document.querySelector(".dismiss-btn");

function isValidEmail(email) {
return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email);

@Tamir198 Tamir198 Nov 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex should be saved inside a constant variable for example EMAIL_REGEX

}

emailInput.addEventListener("input", function () {
const email = emailInput.value.trim();

@Tamir198 Tamir198 Nov 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add ? to make the value null safe


if (email === "") {
emailInput.classList.remove("error");
errorText.style.visibility = "hidden";

@Tamir198 Tamir198 Nov 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dont want to style directly from here, use css and class named like above

return;
}

if (!isValidEmail(email)) {
emailInput.classList.add("error");
errorText.style.visibility = "visible";
} else {

@Tamir198 Tamir198 Nov 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add a return statement in here for early return like you did before

emailInput.classList.remove("error");
errorText.style.visibility = "hidden";
}
});

// לחיצה על Subscribe
subscribeBtn.addEventListener("click", function () {
const email = emailInput.value.trim();

if (!isValidEmail(email)) {
emailInput.classList.add("error");
errorText.style.visibility = "visible";
return;
}

successEmailSpan.textContent = email;
mainContainer.style.display = "none";
successContainer.style.display = "flex";
});

// לחיצה על Dismiss חוזרים למסך הראשי
dismissBtn.addEventListener("click", function () {
successContainer.style.display = "none";

@Tamir198 Tamir198 Nov 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take all of my comments and apply them into this file as well

mainContainer.style.display = "flex";

emailInput.value = "";
emailInput.classList.remove("error");
errorText.style.visibility = "hidden";
});
Loading