please check it #1
Conversation
There was a problem hiding this comment.
Hoi Oletka,
Het was leuk om je te reviewen. Echt alles al supergoed gedaan. Ik was vooral onder de de indruk dat je ook al werkte met vh en vw. Sjiek. Ik heb per regel commentaar gegeven, dat kan je allemaal nalezen. Hier alvast de algemeenheden:
Over de pull request zelf:
- leuk dat je een berichtje had achtergelaten.
- Je branch heette uitwerking. Ik vond het wat verwarrend (2 schermen naast elkaar, 1 met NOVI uitwerking en 1 die van jou) . Want de NOVI uitwerking heet ook uitwerking. Ze gaven aan dat je branch meestal eerst feature noemt en dan een naam. Als je feature/uitwerkingen had gekozen, dan was dat onderscheid beter. En duidelijk dat het afwijkt van de main branch.
Opdrachten zelf
Ik heb alles per regel weergegeven. Echter globaal een aantal opmerkingen:
- In de request kon ik zien dat je de bovenste twee rijen had verwijderd. Heel raar, bij mij stond in het groen nog steeds 1 zin met " schrijf hier je CSS style". Geen idee of dat bij mij ligt of dat er iets mis is gegaan.
- Deel 1 : alles klopte, echter je code style kon aandacht krijgen. Er zit vaak geen ruimte tussen de elementen die je styled. Of de sluit } was niet meteen na laatste styling. Kijk maar even bij de commentaren bij de regels zelf.
- Deel 2: uiteindelijk resultaat bij alle 5 de opdrachten waren goed. Toch een paar aandachtspunten:
a) de code style: tussen gestylde elementen geen ruimte
b) functionaliteit: bij opdracht 4 was je styling-order onlogisch en lastig te lezen. Want je ging eerst de flex-items stylen en daarna kwam pas de boxen.
c) in de NOVi uitwerkingen zie je dat ze voor header en footer px gebruiken en rest ook vh of vw. Geen idee wat de reden daarvoor is, weet ook niet of dat je is opgevallen?. Omdat jij voor header-footer ook vh /vw gebruikt, echter dacht het toch even onder je aandacht te brengen.
Deel 3: prima gedaan. Ik zou alleen oppassen om niet teveel flex-boxen te declareren als het niet nodig is. Echter resultaat telt boven zoiets hoor.
| display: flex; | ||
| flex-direction: row; | ||
| justify-content: space-between; | ||
|
|
There was a problem hiding this comment.
Ik zou deze spatie weghalen. Zodat code netjes is.
| .opdracht-2 .flex-container { | ||
| display: flex; | ||
| flex-direction: row; | ||
|
|
There was a problem hiding this comment.
Ik zou deze spatie weghalen. Zodat code netjes is.
| flex-direction: row; | ||
| justify-content: space-between; | ||
| } | ||
| .opdracht-3 .left { |
There was a problem hiding this comment.
Fijn dat hier de code per element correct is, dus geen spatie voor het laatste }. Alleen nu mis ik een spatie tussen de elementen. Zodat het meteen duideljk is dat dit nieuw element is.
Geldt voor heel opdracht 3. Ze staan allemaal meteen opeenvolgend.
| display: flex; | ||
| flex-direction:row; | ||
| flex-wrap: wrap; | ||
| gap: 11px; |
There was a problem hiding this comment.
small detail. In uitwerking opdracht kozen ze voor 10px ;-)
| align-items: center; | ||
|
|
||
| } | ||
| .opdracht-5 .item-1 { |
There was a problem hiding this comment.
Jouw oplossing vind ik logisch om de items een class te geven. Heb ik zelf ook gedaan. De gevorderde codeerder kan ook denken aan deze oplossing die NOVI meegeeft :
" /Met de selector nth-of-type(getal) kun je het "zoveelste" element van dat type aanspreken/
.opdracht-5 .flex-column:nth-of-type(1) {
align-items: flex-start;
} "
| text-transform: uppercase; | ||
| } | ||
| .navigation { | ||
| display:flex; |
There was a problem hiding this comment.
ik neem aan dat je de flex-direction niet schrijft omdat je de default nodig hebt? De row. DAt klopt ook
| flex-grow: 0; | ||
| } | ||
|
|
||
| h2 { |
There was a problem hiding this comment.
dit is de company name, had ik gelukkig onthouden. Mochten er meer h2 zijn, dan handig deze een class te geven. ik mis nog font-weight: bold;
| text-align: center; | ||
| min-height: 100vh; | ||
| } | ||
| .header-content { |
There was a problem hiding this comment.
het werkt. Alleen de vraag is of je hier een flex-box nodig hebt. In de NOVI uitwerking maken ze van die h1 en h3 titels geen flex-box. Dat komt pas bij de 3 articles.
NOVI heeft alleen ="about-us-section" section met dus die 2 titels, dan een inner-container met de articles. Dan alleen padding nodig voor deze twee titels h1 en h3. Jij hebt een section-content met een header content en content.
|
|
||
| } | ||
| .content { | ||
| display: flex; |
There was a problem hiding this comment.
ga ervan uit dat de default richting wil, wat klopt
| padding: 40px; | ||
| } | ||
| article { | ||
| display: flex; |
There was a problem hiding this comment.
een max height was opgegeven. Dus height: 350px;
2 participants
Hi! Kunt u alstublieft mijn huiswerk controleren?