-
-
Notifications
You must be signed in to change notification settings - Fork 621
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
NW6 | RABIA AVCI | HTML-CSS | MODULE-PROJECT | WEEK-1 #647
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for cyf-module-project-html-css ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 project was done properly but, double-check the below notes:
1)The contact inside main class should be arranged for smaller screens.
2)The collapsible navbar.
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.
Hi,
Good work not too much to moan about. Do you want to discuss flex in more detail at some point?
:root { | ||
--dark-gray: rgb(75, 75, 75); | ||
--light-gray: rgb(171, 171, 171); | ||
--lighter-gray: rgb(249, 249, 249); |
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.
Using vars is very good :)
} | ||
|
||
.store { | ||
display: grid; |
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.
I'm glad to see you using grid and flex for diffrent purposes, They make sense where you are using them
|
||
.copyright { | ||
color: var(--light-gray); | ||
} |
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.
Overall, this looks really solid and makes sense :)
</body> | ||
<header class="header"> | ||
<a href="index.html"> <img src="img/karma-logo.svg" alt="karma-logo" /> </a> | ||
<nav class="nav-menu"> |
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.
Lovely semantic HTML
<div class="col-4 flex-column"> | ||
<label for="city">City *</label> | ||
<select id="city" name="city"> | ||
<option value="" disabled selected>Select your city...</option> |
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.
I really like the use of disabled and selected together, it's a nice touch that you can't select the empty option
Thank you for your review. I think you're talking about the level-3 part which is next week's backlog. https://github.com/CodeYourFuture/HTML-CSS-Module-Project/tree/master/level-3 am I right? |
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.
Good use for the tags, but it would be more understandable if you used more descriptive sentences inside the .
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.
Excellent use for the alt attribute.
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.
Good use for the alt attribute.
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.
Little sugestion about the hover effect on "place my order".
|
||
.primary-btn:hover { | ||
color: var(--orange); | ||
background-color: var(--white); |
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.
I believe the best option here, in the case of keeping the white background with orange text, is to add an outline/border to make the button's boundaries visible. As it is, the button blends in with the background of the page, and only the text is visible.
Volunteers: Are you marking this coursework? You can find a guide on how to mark this coursework in
HOW_TO_MARK.md
in the root of this repositoryYour Details
Homework Details
Notes
What did you find easy?
Design and Content (images, headers) was ready.
What did you find hard?
Layout setup
What do you still not understand?
Flex layout details such us wrap reverse
Any other notes?