Dev #25
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| function inputFocusOut(){ | ||
|
Member
There was a problem hiding this comment. This function name does not indicate what's its doing, can you think of a better name? |
||
| var form = document.getElementsByTagName("form"); | ||
|
Member
There was a problem hiding this comment. Why do you need to get all of those elements? and not just the one you need? Also, what if tomorrow the tag will get replaced into div or something lese? consider get the element by its class name |
||
| var input_mail = document.forms["form"]["email"] | ||
| if (!input_mail.value.toLowerCase() | ||
| .match( | ||
| (/^[A-Za-z\._\-0-9]*[@][A-Za-z]*[\.][a-z]{2,4}$/) | ||
|
Member
There was a problem hiding this comment. This regex should be saved inside constant variable |
||
| )){ | ||
| form[0].getElementsByClassName("error")[0].style.display = "block"; | ||
|
Member
There was a problem hiding this comment. We never change the display from js files, change should be from the css files |
||
| input_mail.classList.add('input-error'); | ||
| } | ||
| else{ | ||
| form[0].getElementsByClassName("error")[0].style.display = "none"; | ||
| input_mail.classList.remove('input-error'); | ||
| } | ||
| } | ||
|
|
||
| function dismiss(){ | ||
| window.location.href="./"; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,208 @@ | ||
| :root { | ||
| --Blue800: hsl(234, 29%, 20%); | ||
| --Blue700: hsl(235, 18%, 26%); | ||
| --Grey: hsl(0, 0%,58%); | ||
| --White: hsl(0, 0%, 100%); | ||
| --accent: hsl(4, 100%, 67%); | ||
| font-family: "Roboto"; | ||
| color: var(--Blue800); | ||
|
|
||
| font-weight: 400; | ||
| font-size: 16px; | ||
| } | ||
|
|
||
| body{ | ||
| background-color: var(--Blue700); | ||
| display: flex; | ||
| justify-content: center; | ||
| justify-items: center; | ||
| margin: 0; | ||
| height: 100vh; | ||
| width: 100vw; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .popup{ | ||
| height: fit-content; | ||
|
Member
There was a problem hiding this comment. If you remove this line the behavior will change? |
||
| background-color: white; | ||
| border-radius: 32px; | ||
| display: grid; | ||
| padding: 20px; | ||
| align-items: center; | ||
| grid-template-columns: auto auto; | ||
| } | ||
|
|
||
| #thanks{ | ||
| width: 360px; | ||
| padding: 48px 60px; | ||
| display: block; | ||
| } | ||
|
|
||
| .main{ | ||
| display: flex; | ||
| flex-direction: column; | ||
| justify-content: center; | ||
| } | ||
|
|
||
| .main2{ | ||
| display: grid; | ||
| grid-template-rows: auto 60px; | ||
| height: -webkit-fill-available; | ||
| } | ||
|
|
||
| grid{ | ||
| display: grid; | ||
| align-items: center; | ||
| justify-content: center; | ||
| height: auto; | ||
| } | ||
|
|
||
| .left{ | ||
| padding: 40px; | ||
| } | ||
|
|
||
| .title{ | ||
| font-size: 54px; | ||
|
Member
There was a problem hiding this comment. Will this be responsive on smaller devices? |
||
| font-weight: 900; | ||
| margin: 8px 0px; | ||
| } | ||
|
|
||
| .text{ | ||
| margin: 24px 0px 36px 0px; | ||
| line-height: 1.2lh; | ||
| } | ||
|
|
||
| .row{ | ||
| padding: 28px 0px 12px 0px; | ||
| font-size: 12px; | ||
| font-weight: 700; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| } | ||
|
|
||
| .error{ | ||
| color: var(--accent); | ||
| display: none; | ||
|
Member
There was a problem hiding this comment. Why do you need display none and color? one is canceling the other |
||
| } | ||
|
|
||
| input{ | ||
| width: -webkit-fill-available; | ||
| height: 56px; | ||
| border-radius: 8px; | ||
| border-color: var(--Grey); | ||
| font-weight: 500; | ||
| border-width: 1px; | ||
| font-size: 16px; | ||
| border-style: solid; | ||
| padding: 0px 16px; | ||
| margin: 0px 0px 28px 0px; | ||
| } | ||
|
|
||
| .input-error{ | ||
| border-color: var(--accent); | ||
| background-color: hsl(4, 100%, 67%,10%); | ||
| color: hsl(4, 100%, 67%); | ||
| } | ||
|
|
||
|
|
||
| .bulletPoint{ | ||
| color: var(--Blue800); | ||
| font-family: "Roboto"; | ||
| display: flex; | ||
| gap: 16px; | ||
| padding: 8px 0px; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .circle:hover{ | ||
| background-color: hsl(25, 97%, 53%); | ||
|
Member
There was a problem hiding this comment. colors should come from css variables |
||
| color: hsl(213, 19%, 18%); | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| .circle:hover:active{ | ||
| background-color: hsl(25, 85%, 44%); | ||
| } | ||
|
|
||
| .circle:disabled{ | ||
| background-color: white; | ||
| color: #272E38; | ||
|
Member
There was a problem hiding this comment. Same here and for all the colors Also be consistent, |
||
| } | ||
|
|
||
|
|
||
|
|
||
|
|
||
| .submit{ | ||
| background-color: var(--Blue800); | ||
| height: 56px; | ||
| width: -webkit-fill-available; | ||
| border-radius: 8px; | ||
| font-weight: 700; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| color: white; | ||
| font-size: 16px; | ||
| border-width: 0px; | ||
| } | ||
|
|
||
| .submit:hover{ | ||
| background-image: linear-gradient(to left,#F27248,#F25D7C); | ||
| color: white; | ||
| cursor: pointer; | ||
| filter: drop-shadow(0px 16px 12px hsla(4, 100%, 67%, 0.4)); | ||
| } | ||
|
|
||
|
|
||
| .moblie_photo{ | ||
| display: none; | ||
| } | ||
|
|
||
|
|
||
| @media screen and (max-width: 880px) { | ||
| body{ | ||
| background-color: white; | ||
| } | ||
| .popup{ | ||
| width:100%; | ||
| height: -webkit-fill-available; | ||
| border-radius: 0px; | ||
| display: flex; | ||
| flex-direction:column; | ||
| padding: 0px; | ||
| align-items: center; | ||
| grid-template-columns: auto auto; | ||
| } | ||
| .left{ | ||
| padding: 0px 20px 40px 20px; | ||
| height: 100%; | ||
| display: flex; | ||
| flex-direction: column; | ||
| justify-content: space-between; | ||
| } | ||
| .title{ | ||
| font-size: 40px; | ||
| } | ||
| .bulletPoint{ | ||
| align-items: flex-start; | ||
| padding: 8px 0px; | ||
| } | ||
| .desktop_photo{ | ||
| display: none; | ||
| } | ||
| .moblie_photo{ | ||
| display: inline; | ||
| width: 100%; | ||
| margin: 0px 0px 20px 0px; | ||
| } | ||
| .upper{ | ||
| display: flex; | ||
| flex-direction: column; | ||
| align-items: flex-start; | ||
| justify-content: center; | ||
| } | ||
|
|
||
| #thanks{ | ||
|
Member
There was a problem hiding this comment. Why do you need id? and not class? |
||
| width: 100%; | ||
| padding: 40px 20px; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| <!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 --> | ||
| <link rel="stylesheet" href="style.css"> | ||
|
Member
There was a problem hiding this comment. Never push comments, remove this because this is not adding anything to the code |
||
| <link rel="icon" type="image/png" sizes="32x32" href="./assets/images/favicon-32x32.png"> | ||
| <script src="script.js"></script> | ||
|
|
||
|
|
||
| <title>Frontend Mentor | Newsletter sign-up form with success message</title> | ||
|
|
||
| <!-- Feel free to remove these styles or customise in your own stylesheet 👍 --> | ||
|
|
||
| </head> | ||
| <body> | ||
| <div class="popup" id="thanks"> | ||
|
Member
There was a problem hiding this comment. Why do you need both id and class? |
||
| <div class="main2"> | ||
|
Member
There was a problem hiding this comment. Can you find a better name for it ratter than |
||
| <div class="upper"> | ||
| <img src="./assets/images/icon-success.svg" height="60px"> | ||
| <div class="title"> | ||
| Thanks for subscribing! | ||
| </div> | ||
| <div class="text"> | ||
|
Member
There was a problem hiding this comment. text is very general, can you find a better name? |
||
| A confirmation email has been sent to | ||
| <div style="font-weight: 700; display: inline;"> | ||
|
|
||
| ash@loremcompany.com. | ||
| </div> | ||
| Please open it and click the button inside to confirm your subscription. | ||
| </div> | ||
| </div> | ||
| <button class="submit" onclick="dismiss()"> | ||
| Dismiss message | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </body> | ||
| </html> | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 id and class?
use the form tag instead if you want to declare form
we never style inline, only from css files
Apply this changes for every place in your code