London Class 9_Turing - HTML/CSS - Week 1#510
Conversation
|
finished = last updated in HTML/CSS week1 |
|
form use grid = added grid in the form |
samhoooo
left a comment
There was a problem hiding this comment.
Thanks for your work, I appreciate it. Here are some comments.
| */ | ||
|
|
||
| header { | ||
| height: 80px; |
There was a problem hiding this comment.
The font size in header may change, would it be better if we use relative unit (em/rem) to fix height?
There was a problem hiding this comment.
it will update next week (HTML/CSS week3)
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| padding: 0 2rem; |
There was a problem hiding this comment.
This is a good use of relative unit to padding!
| display: flex; | ||
| gap: 2.5rem; | ||
| list-style-type: none; |
There was a problem hiding this comment.
This is a good candidate to apply a flex box, nice!
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| gap: 3rem; |
There was a problem hiding this comment.
I also like the use of flex box in here
samhoooo
left a comment
There was a problem hiding this comment.
I can see you have put much effort on this task, keep going! Here are some comments
| .fillForm { | ||
| display: grid; | ||
| grid-template-columns: repeat(8, 1fr); | ||
| grid-template-areas: | ||
| "fn fn fn fn ln ln ln ln" | ||
| "ad ad ad ad ad ad ad ad" | ||
| "a2 a2 a2 a2 a2 a2 a2 a2" | ||
| "cy cy cy cy pc pc pc pc"; | ||
| column-gap: 1rem; |
There was a problem hiding this comment.
A good decision to apply grid in here!
| form { | ||
| display: flex; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| width: 80%; | ||
| width: 75%; | ||
| } | ||
|
|
||
| label { | ||
| font-size: 1rem; | ||
| color: #434242; | ||
| margin-bottom: 0.5rem; | ||
| font-weight: 400; | ||
| } |
There was a problem hiding this comment.
it would be nice to make a class for your form, and specify your label within the form class. So you wouldn't be confused as this can affect other part of your site!
| footer { | ||
| border-top: 1px solid #eee; | ||
| text-align: center; | ||
| margin: 4rem 0; | ||
| padding: 0.5rem 0; | ||
| } | ||
| .footer .icons { | ||
| footer .icons { | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| } | ||
| .footer .icon { | ||
| footer .icon { | ||
| border: 1px solid #ccc; | ||
| border-radius: 50%; | ||
| padding: 1rem; | ||
| margin: 1rem; | ||
| } | ||
| .footer img { | ||
| footer img { | ||
| height: 1.5rem; | ||
| } | ||
| .join { |
There was a problem hiding this comment.
This is good to remove the class from footer, as your site is likely to have only one footer
| button { | ||
| background-color: var(--primary-color); | ||
| border: transparent; | ||
| border-radius: 0.5rem; | ||
| color: #fff; | ||
| font-weight: 400; | ||
| margin-top: 2rem; | ||
| padding: 1rem 2rem; | ||
| } |
There was a problem hiding this comment.
It may be better to make a class for your button, so you wouldn't be confused as this affect all the buttons in your site!
| <nav> | ||
| <ul> | ||
| <li>Meet Karma</li> | ||
| <li style="font-weight: 500">Meet Karma</li> |
There was a problem hiding this comment.
Instead of inline CSS, I guess it would be better if we give this list element an ID, and define its style in the css file? This can make you easier to maintain your site.
Ref: https://stackoverflow.com/questions/2612483/whats-so-bad-about-in-line-css
Volunteers: Are you marking this coursework? You can find a guide on how to mark this coursework in
HOW_TO_MARK.mdin the root of this repositoryYour Details
Homework Details
Notes
What did you find easy?
What did you find hard?
What do you still not understand?
Any other notes?