Feature/image select #268
Feature/image select #268
Conversation
There was a problem hiding this comment.
Zoals @jsebrech al vermeldde in zijn comment lijkt het ons beter om deze code in een aparte component te steken. Ik heb ondertussen wel al een review gedaan op de code. Laat gerust weten als er iets niet helemaal duidelijk is.
| .grid-container { | ||
| display: grid; | ||
| grid-template-columns: 1fr 1fr; | ||
| grid-gap: 10px; |
There was a problem hiding this comment.
Ons grid werkt in veelvouden van 12 en wij hebben enkele spacers voorzien. Ik zou deze regel vervangen door grid-gap: $spacer-xs;
There was a problem hiding this comment.
Heel de grid-container verwijderd en gebruik gemaakt van antwerp flexboxgrid.
|
|
||
| .grid-container { | ||
| display: grid; | ||
| grid-template-columns: 1fr 1fr; |
There was a problem hiding this comment.
Dit werkt goed op mobile, maar ziet er nogal 'bulky' uit vanaf tablet, en zeker op desktop.
There was a problem hiding this comment.
Met behulp van antwerp flexboxgrid ervoor gezorgd dat afhankelijk van de device grootte meer of minder kolommen getoond worden zodat het er minder bulky uit ziet op grote devices.
| border: 3px solid $brand-primary; | ||
|
|
||
| .selectable-image { | ||
| opacity: 0.3; |
There was a problem hiding this comment.
Wij werken liever niet met opacity in de huisstijl. Kan dit op een andere manier?
There was a problem hiding this comment.
Opacity weg gedaan. Een geselecteerde afbeelding wordt nu aangeduid met blauwe rand en het label krijgt een lichtblauwe achtergrond. De afbeelding zelf wordt niet meer voorzien van een lichtblauwe overlay
| grid-gap: 10px; | ||
| } | ||
|
|
||
| .choice { |
There was a problem hiding this comment.
Ik mis hier nog een definitie van een kleine transitie. Zie ook de $animation-normal variable.
| toggleSelected(keyOfChoice: string) { | ||
| if (!this.isDisabled) { | ||
| const selected = this.selectedImageKeys.indexOf(keyOfChoice); | ||
| if (selected < 0 && this.selectedImageKeys.length < this.maxSelectable) { |
There was a problem hiding this comment.
Wil dit zeggen dat je ze nooit allemaal kan selecteren?
There was a problem hiding this comment.
Heb de code klein beetje herschreven zodat duidelijk is wat hier bedoelt wordt. Je kan de keuzes allemaal selecteren.
Deze if clause leest als: als de keuze geslecteerd is EN bij het selecteren van de huidige keuze is er nog niet het maximum aantal items geselecteerd dan ...
| @@ -0,0 +1,10 @@ | |||
| <div class="grid-container"> | |||
There was a problem hiding this comment.
Voor CSS classes graag de guidelines lezen. In het kort: wij hanteren atomic BEM classes. Voor deze component zou een goede benaming bv. zijn: .o-image-select. Andere classes zijn dan bv. .o-image-select__container en .o-image-select__choice is-selected.
There was a problem hiding this comment.
Door gebruik te maken van een checkbox input element zijn deze custom classes niet meer nodig. Css is nu getarget op de standaard input selectors en events.
| @@ -0,0 +1,10 @@ | |||
| <div class="grid-container"> | |||
| <div class="choice" | |||
There was a problem hiding this comment.
Om volledig toegankelijk te zijn, vermoed ik dat je hier beter werkt met checkboxes als achterliggende syntax.
| } | ||
|
|
||
| public setDisabledState(isDisabled: boolean): void { | ||
| this.isDisabled = isDisabled; |
There was a problem hiding this comment.
Ik zie hier nergens styling voor, dus er is nu volgens mij geen onderscheid met een enabled optie.
There was a problem hiding this comment.
Css toegevoegd zodat bij disabled state een standaard cursor wordt getoond en geen 'pointer'.
| } | ||
|
|
||
| toggleSelected(keyOfChoice: string) { | ||
| if (!this.isDisabled) { |
There was a problem hiding this comment.
FYI: deze regel zou niet nodig zijn als je de checkbox syntax gebruikt.
There was a problem hiding this comment.
Is nu inderdaad niet meer nodig :-D
PR Checklist
This PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Adds an image select component, comparable to the selectable list and search-filter components.
Does this PR introduce a breaking change?
Other information
Resolved issues