Funda project | pull request | Larissa Mikkers #21
Conversation
nav bijna klaar, alleen dropdown menu's + mobiel menu
div images onder de header foto toegevoegd met overflow-x scroll, zodat je er door heen kunt scrollen
werk nog niet optimaal, vooral met responsive niet
werkt nog niet (hier nog naar kijken)
moest helaas, anders pakt hij de width niet responsive... als ik meer tijd had had ik dit anders opgelost
There was a problem hiding this comment.
Hi Larissa,
Je hebt goed werk afgeleverd voor deze opdracht! Een paar dingen die er boven uit sprongen:
- Je hebt het design perfect overgenomen
- Je hebt grid op de juiste manier toegepast
- Je hebt invoervelden op de juiste manier van labels voorzien
- Je hebt je werk gestructureerd opgeleverd per commit, voorzien van goede commit messages
Al met al netjes gedaan 👍
| </a> | ||
|
|
||
| <!-- pas zichtbaar op mobiel --> | ||
| <a class="mobileMenu" href="#"> |
There was a problem hiding this comment.
Dit is een link met alleen een icoon. Aangezien hier geen text in staat is deze dus niet accessible voor screen readers. Dit artikel heeft een aantal oplossingen hiervoor: https://www.sarasoueidan.com/blog/accessible-icon-buttons/
| <article class="objectImages"> | ||
|
|
||
| <div class="mainImage"> | ||
| <a href="#"><img src="./assets/hoofdImage.jpg" alt=""></a> |
There was a problem hiding this comment.
Ook hier is het een goed idee om een beschrijving toe te voegen van de image in de alt tag zodat screen readers de afbeeldingen voor kunnen lezen.
| </div> | ||
|
|
||
| <div> | ||
| <button><i class="add"></i></button> |
There was a problem hiding this comment.
hier zijn weer buttons met alleen een icoon. Denk aan de accessibility hiervan.
There was a problem hiding this comment.
Goed gedaan! Een hele complete opdracht. En toch heb je ook de ruimte gehad om nette commit messages te schrijven, een check-list bij te houden en het live te zetten op een URL. Je zet een goed voorbeeld voor de rest. Heel succes met de eindpresentatie/beoordeling en de rest van de opleiding! 🚀
|
|
||
| ## Checklist pagina | ||
|
|
||
| - [x] Buurt blokje |
There was a problem hiding this comment.
Mooi om te zien hoe georganiseerd je te werk bent gegaan. Je hebt zowel gebruik gemaakt van deze checklist als hele nette commits geschreven met duidelijk beschreven commit messages.
| <a href="#" class="logo"><img src="./assets/fundawonen-logo.svg" alt=""></a> | ||
| <ul class="menu"> | ||
| <li> | ||
| <a href="#">Kopen</a> |
There was a problem hiding this comment.
Voor presentatie doeleinden zou ik deze links wel laten linken naar de bewuste live pagina's, zodat het voelt als een echte app. Fake it till you make it!
| </nav> | ||
|
|
||
|
|
||
| <section class="headerImage"> |
There was a problem hiding this comment.
Mooi om te zien dat je gekozen hebt voor een consistente naamgeving van je classes. Wellicht nog interessant om te kijken naar CSS conventies.
|
|
||
| <h4>Oppervlakten en inhoud</h4> | ||
| <dl> | ||
| <dt class="eenKolom">Gebruiksoppervlakten</dt> |
There was a problem hiding this comment.
Desondanks mijn eerste reactie over consistentie naamgeving, zou ik niet kiezen voor classes in meerdere talen.
| </section> | ||
| </div> | ||
|
|
||
| <span class="line grey"></span> |
There was a problem hiding this comment.
Dit kun je gewoon oplossen met een border i.p.v. een combinatie van HTML en CSS.
|
|
||
| .detailList ul li a { | ||
| display: flex; | ||
| gap: 0.35rem; |
There was a problem hiding this comment.
Deze gap is wel heel minimalistisch waardoor ik me af vraag of je grid nodig hebt of dat dit een styling issue geeft.
| @@ -0,0 +1,1088 @@ | |||
| *{ | |||
There was a problem hiding this comment.
Ondanks dat je in dit project slechts CSS kunt gebruiken is dit bestand wel erg lang. Je had er wellicht voor kunnen kiezen om de verschillende "features" op te splitsen over meerdere CSS bestanden.
Hi!
Hierbij mijn eindoplevering van dit project voor de code review.
Link naar site: https://lmikkers.github.io/funda/
@larsdouweschuitema
@rinux55