-
Notifications
You must be signed in to change notification settings - Fork 0
Huiswerk uitwerkingen #1
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
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.
Hoi Maaike,
Zag er al prima uit volgens mij! In ieder geval bijna zoals het voorbeeld (behalve de breedte van de aside op groot scherm mijns inziens), dus lastig om dan te beoordelen of dingen beter/effectiever kunnen vind ik. Volgens mij hebben we beide sommige dingen weer anders gedaan dan in de uitwerkingen van de docent, haha. Hopelijk heb je wat aan de feedback in ieder geval!
|
||
.navbar-one ul { | ||
display: flex; | ||
justify-content: space-between; |
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.
Zou denk ik nog align-items: center toevoegen, om er zeker van te zijn dat ze verticaal uitgelijnd blijven
|
||
.navbar-two ul { | ||
display: flex; | ||
justify-content: flex-end; |
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.
Hier zou ik ook weer align-items: center toevoegen
display: flex; | ||
justify-content: flex-end; | ||
list-style: none; | ||
} |
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.
Ik zou nog een gap toevoegen tussen de li-items; in het voorbeeld staan ze wat verder uit elkaar
display: flex; | ||
align-items: center; | ||
list-style: none; | ||
} |
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.
Net als hierboven zou ik ook hier een gap toevoegen
.navbar-four-logo { | ||
display: flex; | ||
align-items: center; | ||
order: 1; |
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.
Volgens mij is de order nier nodig om alle items op de goede plek te krijgen; ik denk dat je dat met de rest van je code al wel hebt gecovered!
flex-direction: column; | ||
overflow-y: auto; | ||
flex-basis: 0; | ||
flex-grow: 1; |
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.
Je wilt dat je rechterblokken veel harder groeien dan je aside/navbar, dus ik zou flex-grow hier een stuk groter maken en de flex-grow op de aside op 1 zetten.
padding-block: 100px; | ||
} | ||
|
||
@media (max-width: 768px) { |
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.
Nice!
flex-direction: row; | ||
justify-content: center; | ||
align-items: stretch; | ||
flex: 1 1 260px; |
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.
Wil je een flex grow en shrink op de kaarten? Aangezien je wrap hebt gebruikt, bewegen ze flexibel mee bij verschillende schermgroottes, zonder dat ze zelf van grootte hoeven veranderen.
} | ||
|
||
.card-price img { | ||
max-width: 24px; |
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.
Deze icons zijn nu groter dan de icons in je navbar. Als dat je bedoeling was top, ik zou ze misschien 16px maken in lijn met de andere icons op de pagina.
|
||
.card-price img { | ||
max-width: 24px; | ||
display: block; |
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.
Volgens mij doet dit niks?
Hopelijk is het zo duidelijk en anders hoor ik het graag. Bedankt voor het bekijken van mijn code in html en css.