feat(table): adds single row selection mode for data table #888
Conversation
ogermain-kronos
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
Storybook for this build: https://ds.equisoft.io/pr-888/ |
ogermain-kronos
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
Webapp for this build: https://ds.equisoft.io/pr-888/webapp/ |
| @@ -140,31 +142,55 @@ const ExpandButton = styled(IconButton) <{ $expanded: boolean }>` | |||
| } | |||
| `; | |||
|
|
|||
| function getUtilityColumn<T extends object>(type: UtilityColumnType, t: TFunction<'translation'>): TableColumn<T> { | |||
| const RadioButton = styled.input` | |||
There was a problem hiding this comment.
Ca n'a pas le design du Radio Button
Actuel 
Expected 
On a deja ca dans Radio-Button-Group, il faudrait comme extraire juste le input pis faire un component Radio Button seul.
There was a problem hiding this comment.
On s'inspire du King, I see
| disabled={!row.getCanSelect()} | ||
| onChange={row.getToggleSelectedHandler()} | ||
| onClick={() => { | ||
| table.toggleAllRowsSelected(false); |
There was a problem hiding this comment.
J'ouvre la discussion, mais le radio button a un comportement de toggle presentement, si tu le re-click, ca va le deselectionner. Et je ne sais pas si c'est voulu ou conforme. Dans mon experience je n'ai jamais vu un radio qui peut se deselectionner. En meme temps, on a pas d'autre moyen d'enlever une selection non plus. C'est quoi vos opinions la dessus?
There was a problem hiding this comment.
Je l'ai fait comme ça parce que j'étais pas certain qu'on devait locker le user dans un choix ou non. Je suis ouvert aux propositions.
There was a problem hiding this comment.
Pas une réponse concrète mais un peu de recherche.
Les exemples que j'ai trouvés pour un single select avec bouton radio n'offraient pas de moyen de désélectionner:
https://www.lightningdesignsystem.com/components/data-tables/#Single-row-selection
https://www.naiveui.com/en-US/os-theme/components/data-table#select-single.vue
Celle-là le permet mais elle n'a pas de bouton radio. Ça retire l'ambiguité mais ça communique moins bien la possibilité d'interaction selon moi:
https://datatables.net/examples/api/select_single_row.html
There was a problem hiding this comment.
Yeah le datatables.net m'apparait pas trop clair, j'ai pas l'impression de vraiment selectionner le row. Ca peut confondre avec un action de juste highlighter un row pour faciliter la lecture.
| @@ -214,7 +240,7 @@ export interface TableProps<T extends object> { | |||
| * @default medium | |||
| */ | |||
| rowSize?: RowSize; | |||
| selectableRows?: boolean; | |||
| selectionMode?: SelectionMode; | |||
There was a problem hiding this comment.
C'est un BREAKING CHANGE, il faut ajouter un ! et documenter le breaking change.
BREAKING CHANGE:
- Table prop "selectableRows" is not a boolean anymore.
https://www.conventionalcommits.org/en/v1.0.0/#commit-message-with-both--and-breaking-change-footer
| @@ -214,7 +240,7 @@ export interface TableProps<T extends object> { | |||
| * @default medium | |||
| */ | |||
| rowSize?: RowSize; | |||
| selectableRows?: boolean; | |||
| selectionMode?: SelectionMode; | |||
There was a problem hiding this comment.
On a aussi un expandableRows?: 'single' | 'multiple', le concept est similaire, on devrait garder la meme nomenclature.
| selectionMode?: SelectionMode; | |
| selectableRows?: SelectionMode; |
There was a problem hiding this comment.
C'est débatable. Honnnêtement je trouve que selectionMode est pas mal plus clair sur la fonction de la prop que selectableRows imho.
There was a problem hiding this comment.
Yeah ton point est valide, pour moi c'est pas tant le nom mais plus le fait qu'on pourrait garder une certaine similitude entre les noms. Ca pourrait etre expandableRows qui se renomme a expandMode aussi.
There was a problem hiding this comment.
Par contre je me demande si on ne devrait pas garder row ou rows dans les noms de props, vu que ça affecte les rows et non la table. row(s)Selection et row(s)Expansion?
There was a problem hiding this comment.
LGTM, après avoir changé le style du bouton, comme dit plus haut. ☝️ (#888 (comment))
| onClick={() => { | ||
| table.toggleAllRowsSelected(false); |
There was a problem hiding this comment.
J'ai testé la nouvelle fonctionnalité et dite moi si je sors du scope de la tâche.
Pas que j'ai le parkinson, mais j'ai trouvé ça un peu désagréable de devoir être précis pour clicker sur le bouton radio. Est-ce qui serait pas trop compliquer de toggle quand on click aussi la row ? 🤔
Sa peu-être déjà été discuté, mais je pose quand même la question au cas où. 🤷♂️
There was a problem hiding this comment.
J'aime vraiment la réflexion.
Par contre, disons que la row est clickable au complet:
- s'il y a une cell avec un lien
<a>à l'intérieur, ça peut être tannant lorsque l'utilisateur va clicker sur le lien, ca va trigger le radio button, même chose s'il y a des<button>. - si l'utilisateur veut sélectionner le texte d'une cell pour faire un copier/coller, il va devoir clicker dans la cell pour le sélectionner et ca va trigger le radio button.
Je sais que nous pourrions contourner tout ça d'une manière xyz mais je préfère garder ca le plus simple possible. Ce que nous pourrions faire par contre serait d'avoir la cell clickable au lieu de toute la row.
There was a problem hiding this comment.
+1 pour le cell cliquable.
There was a problem hiding this comment.
Comme on s'enligne pour créer notre propre component pour le radio button, il devrait inclure un élément label comme la checkbox. À ce moment-là, il serait possible d'ajouter un padding au label (même sans texte) pour grossir le hit area. Ça éviterait de faire une passe-passe avec le clic sur la cell.
Je ne pense pas que le label soit un problème pour l'a11y parce que les attribut aria-labelledby ont la priorité sur les éléments <label> lorsque les deux sont présents.
| ); | ||
| } else if (selectionMode === 'single') { | ||
| column.cell = ({ table, row }) => ( | ||
| <RadioButton |
There was a problem hiding this comment.
La radio button devrait avoir un <label> d'associé. S'il n'y a pas de <label> les technologies d'assistances (lecteur d'écran) vont seulement annoncer que c'est un radio button et rien d'autre. Le <label> sert à identifier de quel radio button il s'agit. e.g., si le <label> est 'patate' le lecteur d'écran va annoncer: 'patate, radio button'
Il y a deux options possibles:
-
aria-label
ajouter unaria-label="label du radio button"à l'input.
Tu aurais donc:
<input data-testid="..." type="radio" class="..." aria-label="patate">. -
hidden text
ajouter une classe qui vient cacher visuellement le<label>.
<input id="idInput" data-testid="..." type="radio" class="..."><label for="idInput" class="visually-hidden">patate</label>
Comme mentionné dans un autre commentaire, il faudrait utiliser le composant de radio button du DS. Ça va p-e demander du boulot dans le composant radio-button par contre.
Je vais regarder le travaille qu'il y a à faire sur le composant et si c'est trop de boulot, je vais créer une carte pour y faire les modifications et on reviendra sur ta PR quand les modifications pour le radio button seront fait.
https://equisoft.atlassian.net/browse/DS-1101