daniel aminov's project #10
Conversation
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Hey good work, I left you some comments.
| @@ -0,0 +1,3 @@ | |||
| const AppConstants = { | |||
There was a problem hiding this comment.
Great work, this is a good practice
| @@ -0,0 +1,109 @@ | |||
| @media screen and (min-width: 800px) { | |||
There was a problem hiding this comment.
| .container { | ||
| width: 850px; | ||
| height: 590px; | ||
| padding: 2em; |
There was a problem hiding this comment.
Any reason to mix px and em ?
I would keep consistency with 1 unit as much as possible
| background-image: url("./assets/images/illustration-sign-up-desktop.svg"); | ||
| } | ||
| button { | ||
| height: 25%; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| .container { | ||
| flex-direction: column-reverse; | ||
| width: 320px; | ||
| height: 110vh; |
There was a problem hiding this comment.
Putting aside the differences between the css units, why would you want some container to be more tall then the current view port?
| <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 --> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| <article class="s-container"> | ||
| <img class="check-icon" src="./assets/images/icon-list.svg" /> | ||
| <h1>thanks for subscribing!</h1> | ||
| <p id="article-text"></p> |
There was a problem hiding this comment.
Why do you need id in here? class should work and its got less specificity
There was a problem hiding this comment.
i used id in order to access the html tag in dom and not for css , is it still considered bad practice?
| 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"); |
There was a problem hiding this comment.
You should add null checks in here, for stuff like :
element is not defined
no url params
| const email = urlParams.get(AppConstants.EMAIL_PARAM_KEY); | ||
| const paragraph = document.getElementById("article-text"); | ||
|
|
||
| paragraph.innerHTML = `A confirmation email has been sent to <strong>${email}</strong>. Please open it and click the button inside to confirm your subscription.`; |
There was a problem hiding this comment.
innerHTML considered to be really a bad practice, use textContent instead
There was a problem hiding this comment.
i used innerHtml because i want to use "<strong/" in order to bold one word and it isnt working with textContent.
There was a problem hiding this comment.
Thays because you should make the text bold via css
| }); | ||
|
|
||
| const dismissButton = document.getElementById("return-button"); | ||
| dismissButton.addEventListener("click", function () { |
There was a problem hiding this comment.
You should remove your event lister after you navigate into another page and this listener is no longer needed
2 participants
No description provided.