-
Notifications
You must be signed in to change notification settings - Fork 48
Ports Myriam #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: master
Are you sure you want to change the base?
Ports Myriam #43
Conversation
shubha-rajan
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.
Hi Myriam! I know you didn't have the chance to finish, but from what I see, you're on the right track. A few things:
First, I love how specific your selectors are (using ids instead of classes, specifying the nesting of an element, etc. This is a good habit to have when you want to avoid unexpected weird things happening to new content you might add to the page when updating it later.
That being said, it's ok to use the more general class and element selectors for things that you for the most part want to do across the page. One really good example is removing the bullets from your unordered lists. On this page, you removed them from every ul, so to DRY up your code, you can just declare that all uls will have list-style-none. If you do eventually need a bulleted list, you can add the bullets back for that specific class or id. A good case for using classes are when two things have mostly the same styling. The quotes are a good example of this. They have almost the same styling (except for their grid-area), so you can make a quote class and do most of the styling there, and use the ids only for assigning them their specific grid area.
Can't wait to see what this looks like when you're finished!
| .container{ | ||
| display: grid; | ||
| grid-template: 1fr 2fr 2fr .5fr 2fr 2fr 3.5fr 2fr auto / 2fr 1fr 1fr; | ||
| grid-template-areas: |
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 use of named areas here. this can be super helpful when wrapping your head around complex grid layouts.
| nav { | ||
| grid-area: nav; | ||
| margin: 20px; | ||
| flex-wrap: wrap; |
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.
did you mean to put flex-wrap here? usually flex-wrap lives in whatever ruleset display:flex lives.
| } | ||
|
|
||
|
|
||
| #try_now{ |
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.
it looks like you're using this flexbox to center the text. can i suggest throwing a justify-content:center in here?
| justify-content: space-between; | ||
| } | ||
|
|
||
| #team ul li img{ |
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.
love the specificity here!
Startrly
Congratulations! You're submitting your assignment.
Comprehension Questions