Skip to content

L1 hw #23

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

L1 hw #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
28558f1
add123
RazEl12 Nov 16, 2025
8fd3062
new brnach
RazEl12 Nov 16, 2025
cf81a70
add box
RazEl12 Nov 16, 2025
da3d872
add options list
RazEl12 Nov 23, 2025
be2b49d
add email
RazEl12 Nov 23, 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
38 changes: 29 additions & 9 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
Expand Up @@ -5,6 +5,7 @@
<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">
<link rel="stylesheet" href="styles.css">

<title>Frontend Mentor | Newsletter sign-up form with success message</title>

Expand All @@ -14,11 +15,34 @@
.attribution a { color: hsl(228, 45%, 44%); }
</style>
</head>
<body>
<body style="background-color: hsl(234, 29%, 20%);">
Copy link

@idanpopovich8 idanpopovich8 Nov 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Its better to move the styling to the style.css file.
Keep a single tag and move the background-color into the CSS instead of using an inline style attribute.


<div class="container">
<div class="text">
<div class="title">
<h1>Stay updated!</h1>
</div>
<p>Join 60,000+ product managers receiving monthly updates on:</p>
<div class="options">
<img class="opimg"src="assets/images/icon-list.svg">
Copy link

@idanpopovich8 idanpopovich8 Nov 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Missing alt artibute inside the img label

<span class="data"> Product discovery and building what matters</span><br>
<img class="opimg"src="assets/images/icon-list.svg">
<span class="data"> Product discovery and building what matters</span><br>
Copy link

@idanpopovich8 idanpopovich8 Nov 28, 2025

Choose a reason for hiding this comment

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

All three bullet points currently show the same text, and class name data its not a good name.
try to be more creative next time.

Copy link

@idanpopovich8 idanpopovich8 Nov 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Its not best practice to work with br label for spacing, Consider using marging it will fits good for your needs.

<img class="opimg"src="assets/images/icon-list.svg">
<span class="data"> Product discovery and building what matters</span><br>
</div>
<h1 class="emailTitle">Email addres:</h1>
<input type="text" id="email" name="email" placeholder="Email@company.com"><br><br>
<button class="subbtn">Subscribe to monthly newsletter</button>
</div>
<div class="image">
<img src="assets/images/illustration-sign-up-desktop.svg" alt="Italian Trulli">
</div>
</div>

<!-- Sign-up form start -->

Stay updated!
<!--Stay updated!
Copy link

@idanpopovich8 idanpopovich8 Nov 28, 2025

Choose a reason for hiding this comment

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

we dont leave our comments inside. its better just to delete them : )


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

Expand All @@ -29,24 +53,20 @@
Email address
email@company.com

Subscribe to monthly newsletter
Subscribe to monthly newsletter-->

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

<!-- Success message start -->

Thanks for subscribing!
<!--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
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>
60 changes: 60 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,60 @@
.container{
border-radius: 25px;
margin: 0;
position: absolute;
top: 50%;
left: 50%;
transform: translate(-50%, -50%);
width: 800px;
Copy link

@idanpopovich8 idanpopovich8 Nov 28, 2025

Choose a reason for hiding this comment

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

It’s not recommended — and even considered incorrect — to use fixed pixel sizes for a div. If the user changes their screen size, the entire layout can break
try to use percentages or Vh and Vw

height:600px;
display: grid;
background-color: white;
grid-template-columns: 1fr 1fr;
border: 1px solid white;
}



.container img {
width: 95%;
Copy link

@idanpopovich8 idanpopovich8 Nov 28, 2025

Choose a reason for hiding this comment

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

Nice job!

height: 100%;
object-fit: fill;
}

.text {
padding-left: 40px;
padding-block: 80px;
padding-right: 40px;
}
.title{
font-size: 25px;
}

.opimg{
max-height: 15px;
max-width:25px;
}

.data{
padding-left: 5px;
font-size: 14px;
}

.emailTitle{
font-size: 14px;
}

#email{
display: flex;
width: 300px;
height: 8%;
}
.subbtn{
display: flex;
width: 308px;
Copy link

@idanpopovich8 idanpopovich8 Nov 28, 2025

Choose a reason for hiding this comment

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

same comment as before

height: 8%;
justify-content: center;
align-items: center;
background-color: hsl(234, 29%, 20%);
color: white;
}