Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
53 changes: 27 additions & 26 deletions newsletter-sign-up-with-success-message-main/index.html
Original file line number Diff line number Diff line change
@@ -1,52 +1,53 @@
<!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="stylesheet" href="./styles/style.css">

<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>

<!-- 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>

<title>Newsletter sign-up</title>
</head>

<body>

<!-- Sign-up form start -->
<div class="form-container">
<div class="form-column" id="form-text">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So in here you are using both id and class, you dont really need both of them, you can remain with only class

<h2 id="heading">
Stay updated!
</h2>

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

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!

Product discovery and building what matters
Measuring to ensure updates are a success
And much more!
Email address
email@company.com

Email address
email@company.com
Subscribe to monthly newsletter
</div>

Subscribe to monthly newsletter
<div class="form-column">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

HTML is like a book for developers, and we all know that form label means that we have a form inside so you could use form instead of your div

So its more readable

<div id="desktop-image">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not really the desktop image, this is the container so I would change the name desktop-image-container

<img src="./assets/images/illustration-sign-up-desktop.svg" alt="Sign up illustration">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good work on the alt attribute

</div>
</div>
</div>

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

<!-- Success message start -->

Thanks for subscribing!

A confirmation email has been sent to ash@loremcompany.com.
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>
36 changes: 0 additions & 36 deletions newsletter-sign-up-with-success-message-main/style-guide.md

This file was deleted.

75 changes: 75 additions & 0 deletions newsletter-sign-up-with-success-message-main/styles/style.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
- Mobile: 375px
- Desktop: 1440px

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When you are making a pr do not keep those comments, looks like ai generated comment?

*/

:root {
font-family: 'Roboto', sans-serif;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a great practice

font-size: 16px;
--originalBlue800: hsl(234, 29%, 20%);
--originalBlue700: hsl(235, 18%, 26%);
--originalGrey: hsl(0, 0%, 58%);
--originalWhite: hsl(0, 0%, 100%);
--primaryRed: hsl(4, 100%, 67%);
}

*,
*::before,
*::after {
box-sizing: border-box;
}

body {
background-color: var(--originalBlue800);
width: 98vw;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

any reason not to use 100vw? why would we want the body not to be 100% in width?

height: 98vh;
overflow: hidden;
color: var(--originalBlue800);
display: flex;
align-items: center;
justify-content: center;
flex-direction: column;
}

.form-container {
background-color: var(--originalWhite);
width: 50%;
height: 60%;
border-radius: 25px;
display: flex;
flex-direction: row;
overflow: hidden;
}

.form-column {
flex: 1 1 0;
min-width: 0;

height: 100%;
padding: 1.5rem;

display: flex;
flex-direction: column;
align-items: center;
justify-content: center;
}

#form-text {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In here you are using id instead of classes, any reason for this?
because id got higher specificity than class ?

width: 100%;
}

#desktop-image {
width: 100%;
height: 100%;
display: flex;
align-items: center;
justify-content: center;
overflow: hidden;
}

#desktop-image img {
max-width: 100%;
/* max-height: 100%; */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please do not push commended code, remove it, we got git for seeing history

object-fit: contain;
display: block;
}