Skip to content

daniel aminov's project #10

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
ItsAminov wants to merge 3 commits into
base: main
Choose a base branch
from
Open

daniel aminov's project #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f23333
indeactive commit message
ItsAminov Nov 16, 2025
2b667b0
done
ItsAminov Nov 19, 2025
2ee5052
removed unused filesand folders
ItsAminov Nov 19, 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
3 changes: 3 additions & 0 deletions newsletter-sign-up-with-success-message-main/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
const AppConstants = {
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

Great work, this is a good practice

EMAIL_PARAM_KEY: "email",
};
109 changes: 109 additions & 0 deletions newsletter-sign-up-with-success-message-main/index-styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
@media screen and (min-width: 800px) {
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

This is great, it you didnt knew this you can also :

Image

The image is from the docs

.container {
width: 850px;
height: 590px;
padding: 2em;
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

Any reason to mix px and em ?
I would keep consistency with 1 unit as much as possible

}
.img-side {
background-image: url("./assets/images/illustration-sign-up-desktop.svg");
}
button {
height: 25%;
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

What will you think happen if you will place a new button inside really tall parent?
The button may stretch and be super big and then you will have to override this css rule.

I think that a better approach will be to give this button maybe max-height ?

Do you have any other reason to use % in here?

Copy link
Author

@ItsAminov ItsAminov Nov 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i used it because i only had one button in this page and didnt think about the possibillity of adding another one, but you are right i should have

}
}
@media screen and (max-width: 799px) {
.container {
flex-direction: column-reverse;
width: 320px;
height: 110vh;
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

Putting aside the differences between the css units, why would you want some container to be more tall then the current view port?

padding: 0.4em;
margin: 0;
}
.img-side {
max-height: 35%;
background-image: url("./assets/images/illustration-sign-up-mobile.svg");
}
button,
input {
height: 45%;
}
}
.img-side {
border-radius: 10px;
}
.container > * {
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

I know that for this project this approach may look like a good thing, but generally this makes every future .container class act this way.

Not sure that I would want to force every container item to be flex:1 by default

flex: 1;
}
.container {
margin-top: 1em;
justify-self: center;

background-color: white;
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

You should place css colors inside a css variable


border-radius: 20px;
display: flex;
}
.text-side {
display: flex;
flex-direction: column;
justify-content: space-evenly;
padding: 4em 0.1em 1.5em 0.1em;
gap: 1em;
margin: 0.5em 1.5em 0.5em 1.5em;
}
.text-side > * {
flex-grow: 1;
}

ul {
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

Same comment in here, I know that its fine for the exercise but you will want to not have those "default values" for elements like ul.

For this pr its fine, just some general notes

list-style: none;
display: flex;
flex-direction: column;
justify-content: space-around;
height: 5%;
padding: 0;
margin: 0;
}
li {
gap: 0.4em;
flex: 1;
display: flex;
justify-content: flex-start;
align-items: center;
}
h1 {
align-self: center;
max-height: fit-content;
padding: 0;
margin: 0;
}
.check-icon {
justify-self: flex-start;
width: 5%;
height: 40%;
}
p {
margin: 0;
max-height: fit-content;
}
button {
width: 100%;

border-radius: 10px;
border: none;
font-weight: bold;
color: white;
box-shadow: 2px 2px 2px hsla(5, 9%, 27%, 0.695);
}

input {
width: 98%;
height: 25%;
border-radius: 6px;
}
.form {
margin-top: 1em;
display: flex;
flex-direction: column;
gap: 0.5em;
}
114 changes: 69 additions & 45 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,76 @@
<!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 -->
<head>
<script src="constants.js"></script>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<!-- displays site properly based on user's device -->

<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="icon"
type="image/png"
sizes="32x32"
href="./assets/images/favicon-32x32.png"
/>

<!-- 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>
<title>
Frontend Mentor | Newsletter sign-up form with success message
</title>
<link rel="stylesheet" href="styles.css" />
<!-- Feel free to remove these styles or customise in your own stylesheet 👍 -->
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

I know that those comments came with the template, generally when we are making a pr we wont have any commended code.

Just for general knowledge

<link rel="stylesheet" href="index-styles.css" />
</head>
<body>
<main>
<div class="container">
<div class="text-side">
<h1>stay updated!</h1>
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

Good work on using proper tags in here

<p>join 60000+ product managers reciving monthly updates on:</p>
<ul>
<li>
<img
class="check-icon"
src="./assets/images/icon-list.svg"
/>product discovery and bulding what matters
</li>
<li>
<img
class="check-icon"
src="./assets/images/icon-list.svg"
/>messuring to ensure updates are a sucsses
</li>
<li>
<img class="check-icon" src="./assets/images/icon-list.svg" />and
much more
</li>
</ul>
<form class="form">
<label for="email-input">email adress:</label>
<input
id="email-input"
required
placeholder="devClubDeveloper@colman.ac.il"
type="email"
type="text"
/>
<button type="submit">subscribe to monthly newsletter</button>
</form>
</div>
<div class="img-side"></div>
</div>
</main>

<!-- Sign-up form start -->
<footer>all rights belong to aminov.inc®</footer>
<script>
const form = document.querySelector("form");
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

This could be places inside js file and not directly in here

const container = document.querySelector(".container");
const emailInput = document.getElementById("email-input");

Stay updated!
form.addEventListener("submit", function (event) {
event.preventDefault();

Join 60,000+ product managers receiving monthly updates on:

Product discovery and building what matters
Measuring to ensure updates are a success
And much more!

Email address
email@company.com

Subscribe to monthly newsletter

<!-- Sign-up form end -->

<!-- Success message start -->

Thanks for subscribing!

A confirmation email has been sent to ash@loremcompany.com.
Please open it and click the button inside to confirm your subscription.

Dismiss message

<!-- 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>.
</div>
</body>
</html>
window.location.href = `successPage.html?${AppConstants.EMAIL_PARAM_KEY}=${emailInput?.value}`;
});
</script>
</body>
</html>
51 changes: 51 additions & 0 deletions newsletter-sign-up-with-success-message-main/styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
html {
height: 100vh;
width: 96vw;
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

Any reason for the html not take full width by default?

}
footer {
background-color: rgb(155, 150, 150);
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

same comment here as well about the css color that should come from variable

display: flex;
align-items: center;
justify-content: center;
padding: 0;
margin: 0;
}
body {
height: fit-content;
min-height: 100%;
min-width: 100%;
box-sizing: border-box;
display: grid;
grid-template-columns: 1fr;
grid-template-rows: 97% 3%;
}
main {
height: 100%;
width: 100%;
background-color: rgb(230, 227, 227);
}

div {
margin: 0;
padding: 0;
}

label {
font-weight: bold;
}

button {
background-color: hsl(224, 64%, 21%);
color: white;
}

button:hover {
cursor: pointer;
box-shadow: 4px 4px 4px hsla(5, 9%, 27%, 0.695);
background-image: linear-gradient(
to right,
hsl(354, 100%, 67%),
hsl(4, 100%, 67%),
hsl(15, 100%, 67%)
);
}
60 changes: 60 additions & 0 deletions newsletter-sign-up-with-success-message-main/success-styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
@media screen and (min-width: 800px) {
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

In this file i only have the comments above to say, nothing new.
So you can apply the above comments in this file as well

article {
width: 300px;
height: 340px;
padding: 2.5em;
}
button {
max-height: 20%;
}
}
@media screen and (max-width: 799px) {
article {
width: 70vw;
flex-direction: column-reverse;
height: 100%;
padding: 1.5em;
justify-content: flex-start;
margin: 0;
}
button {
margin-top: 40vh;
max-height: 7%;
}
}

article > * {
flex: 1;
}
article {
gap: 2em;
justify-self: center;
display: flex;
flex-direction: column;
border-radius: 30px;
box-shadow: 0 0 10px rgba(0, 0, 0, 0.1);
background-color: rgb(255, 255, 255);
margin-top: 4rem;
max-height: fit-content;
}
button {
border: none;
border-radius: 10px;
padding: 0.75rem 1.5rem;
cursor: pointer;
font-size: 1rem;
width: 100%;
}
img {
width: 15%;
max-height: fit-content;
}
h1 {
margin: 0;
max-height: fit-content;
font-size: 45px;
}
p {
max-height: fit-content;
margin: 0;
}
46 changes: 46 additions & 0 deletions newsletter-sign-up-with-success-message-main/successPage.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<!DOCTYPE html>
<html lang="en">
<head>
<script src="constants.js"></script>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<!-- displays site properly based on user's device -->
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

Looks like AI code, and that ok we all are using AI, just remove his comments.
if not feel free to tell me that im wrong

Copy link
Author

@ItsAminov ItsAminov Nov 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

you are incorrect on this one it, this comment was in the code i forked and forgot to delete.
all the code here was written by me without ai code generation but i did use it for searching.

<link rel="stylesheet" href="styles.css" />
<link
rel="icon"
type="image/png"
sizes="32x32"
href="./assets/images/favicon-32x32.png"
/>
<link rel="stylesheet" href="success-styles.css" />
<title>
Frontend Mentor | Newsletter sign-up form with success message
</title>
</head>
<body>
<main>
<article class="s-container">
<img class="check-icon" src="./assets/images/icon-list.svg" />
<h1>thanks for subscribing!</h1>
<p id="article-text"></p>
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

Why do you need id in here? class should work and its got less specificity

Copy link
Author

@ItsAminov ItsAminov Nov 29, 2025

Choose a reason for hiding this comment

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

i used id in order to access the html tag in dom and not for css , is it still considered bad practice?

<button id="return-button">dismiss msg</button>
</article>
</main>

<footer>all rights belong to aminov.inc®</footer>
<script>
document.addEventListener("DOMContentLoaded", function () {
const urlParams = new URLSearchParams(window.location.search);
const email = urlParams.get(AppConstants.EMAIL_PARAM_KEY);
const paragraph = document.getElementById("article-text");
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

You should add null checks in here, for stuff like :

element is not defined
no url params


paragraph.innerHTML = `A confirmation email has been sent to <strong>${email}</strong>. Please open it and click the button inside to confirm your subscription.`;
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

innerHTML considered to be really a bad practice, use textContent instead

Copy link
Author

@ItsAminov ItsAminov Nov 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i used innerHtml because i want to use "<strong/" in order to bold one word and it isnt working with textContent.

Copy link
Member

@Tamir198 Tamir198 Nov 29, 2025

Choose a reason for hiding this comment

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

Thays because you should make the text bold via css

});

const dismissButton = document.getElementById("return-button");
dismissButton.addEventListener("click", function () {
Copy link
Member

@Tamir198 Tamir198 Nov 26, 2025

Choose a reason for hiding this comment

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

You should remove your event lister after you navigate into another page and this listener is no longer needed

window.location.href = "index.html";
});
</script>
</body>
</html>