-
Notifications
You must be signed in to change notification settings - Fork 62
Sandra - Business site #43
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
Great job! It looks really professional and very well-structured. You have done a great job with the responsive design, love the different sizes of the "cards". I also really like that you added the detail with no-scroll while in the hamburger menu. Nice job! I will comment on some specific lines in the code soon. Again, really good job! You sould be proud of making this site! |
| box-shadow: 0 6px 10px rgba(0, 0, 0, 0.25); | ||
| } | ||
|
|
||
| @media (min-width: 668px) { |
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.
You have made several @media (min-width: 668px). If you add all the code to one of the instead it would be a bit easier to maintain and the code looks cleaner. Just to reduce duplications.
| color: white; | ||
| } | ||
|
|
||
| @media (min-width: 668px) { |
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.
Here it is again
| background-color: #b5bca8; | ||
| } | ||
|
|
||
| @media (min-width: 668px) { |
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.
You have made several @media (min-width: 668px). If you add all the code to one of the instead it would be a bit easier to maintain and the code looks cleaner. Just to reduce duplications.
Will comment on the others as well so you can find them easier :)
| top: 50%; | ||
| } | ||
|
|
||
| @media (min-width: 668px) { |
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.
You have made several https://github.com/media (min-width: 668px). If you add all the code to one of the instead it would be a bit easier to maintain and the code looks cleaner. Just to reduce duplications.
Will comment on the others as well so you can find them easier :)
| hamburger.addEventListener("click", () => { | ||
| navLinks.classList.toggle("active") | ||
| hamburger.classList.toggle("open") | ||
| body.classList.toggle("no-scroll") |
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.
Great feature! Will definitely look into adding no scroll to my own site!
| </div> | ||
| <p>© 2025 Skin Expert. All rights reserved.</p> | ||
| </footer> | ||
| <script src="script.js"></script> |
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.
Maybe consider adding <script src="script.js"></script> to the beginning of the document, in the head, where you have the link to the css file as well. Makes it a bit easier to find.
| </div> | ||
| <p>© 2025 Skin Expert. All rights reserved.</p> | ||
| </footer> | ||
| <script src="script.js"></script> |
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.
Maybe consider adding <script src="script.js"></script> to the beginning of the document, in the head, where you have the link to the css file as well. Makes it a bit easier to find.
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.
The code looks very clean in the html files! Great job!
HIPPIEKICK
left a comment
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.
Really good job with the JS, nice to see you do some extras like the slides ⭐ Remember to be consistent with whether or not you’re using semicolons.
PS. I like the design - looks very professional and aligned. However, it would be nice if the buttons had the same font as the rest of the page 😇
https://skinexpert.netlify.app/