Skip to content
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

feat(Table): add headerAriaLabel prop for header columns #696

Merged
merged 10 commits into from
Feb 2, 2024

Conversation

alexbrillant
Copy link
Contributor

@alexbrillant alexbrillant commented Jan 4, 2024

@alexbrillant alexbrillant requested a review from a team as a code owner January 4, 2024 22:58
Copy link

github-actions bot commented Jan 4, 2024

Storybook for this build: https://ds.equisoft.io/pr-696/

@savutsang
Copy link
Contributor

savutsang commented Jan 8, 2024

Dans JIRA, ca mentionne que le aria-label est seulement applicable si la colonne n'a pas de texte dedans. Et aussi avoir un avertissement (j'imagine console.warn?) s'il y a un aria-label pour une colonne qui contient du texte.

Edit:
A ce que je sache, on ne pourrait pas toujours detecter si c'est un string (pas pendant le rendering React), puisque le dev a la possibilite de render un custom Component ou custom HTML aussi, et dans ces cas ci, on ne saurait pas si c'est un string ou non. Je ne pense pas que ca fait du sens de mettre ces conditions si le code ne marche que la moitie du temps.

Faudrais revoir les requirements du billet, je pense a les enlever, laisser ca au bon jugement des devs de mettre un aria seulement sur les columns qui ont de besoin.

@LarryMatte

savutsang
savutsang previously approved these changes Jan 9, 2024
@savutsang savutsang dismissed their stale review January 9, 2024 16:36

Attendre la resolution

@alexbrillant
Copy link
Contributor Author

@savutsang je pense que c'est mieux de laisser les dev décider si ils veulent un arialabel ou non sur chacune des colonnes.

@savutsang
Copy link
Contributor

savutsang commented Jan 10, 2024

@savutsang je pense que c'est mieux de laisser les dev décider si ils veulent un arialabel ou non sur chacune des colonnes.

Yeah c'est juste de confirmer avec @LarryMatte si c'est ok qu'on ne fera pas ca. Sinon a voir si on doit trouver une autre solution, ou merger juste ceci et creer un autre billet pour ameliorer ca plus tard.

@LarryMatte
Copy link
Contributor

Yeah c'est juste de confirmer avec @LarryMatte si c'est ok qu'on ne fera pas ca. Sinon a voir si on doit trouver une autre solution, ou merger juste ceci et créer un autre billet pour améliorer ca plus tard.

Idéalement, je ne laisserais pas ça dans la cours des dev parce qu'ils ne le mettront juste pas 90% du temps.
Si c'est quelque chose qui peut être complexe à faire, je vais créer une autre tâche pour améliorer plus tard, question de garder une pas pire vélocité.

Questions:

  • Est-ce que nous pouvons quand même avoir un warning pour les devs s'ils décident de ne pas ajouter le aria-label?
  • Si une colonne de la table est utilisée sans aria-label, c'est quoi les conséquences lorsque la mise à jour va se faire advenant le cas où nous fixons l'issue dans le futur i.e., qu'un aria-label est obligatoire s'il n'y a pas de texte dans le column header?

@savutsang
Copy link
Contributor

savutsang commented Jan 10, 2024

Pour emettre des warnings, il faut pouvoir detecter si le contenu du TH est un string ou pas. Et c'est le problematique ici.

// Si le dev le configure de cette facon, on pourrait le detecter
header: '',
header: 'Prenom',

// Si c'est configure de cette facon, non
// on sais juste que le dev a passe une fonction, mais on ne sait pas ce que la function retourne.
header: () => '',
header: () => 'Prenom',
header: () => <br />,
header: () => <span>Prenom</span>,
header: () => <Checkbox />,
header: () => <ColorfulText text="Prenom" />,

Bon, faque soit on decide d'emette des warnings juste sur ceux qu'on peut detecter, et ignorer le reste. Sinon on pourrait essayer de rouler du code JS APRES que react a render la table (dans un useEffect), qui parcoure la table et detecte les TH problematique (avec innerText genre) et log des warnings en consequence. Ou une meilleure idee?

Je crois que c'est un truc qui a besoin d'un peu d'experimentation, sinon on risque de mal detecter des trucs, pis avoir des faux warnings.

Je ne vois pas d'impact si c'est fait plus tard, ca ne change pas le fonctionnement de la table, il y aura juste des warnings de plus.

@savutsang savutsang deleted the branch dev/DS-936-update-react-table January 25, 2024 15:51
@savutsang savutsang closed this Jan 25, 2024
@savutsang
Copy link
Contributor

savutsang commented Jan 25, 2024

Pourquoi ceci a ete close? J'ai juste efface l'ancien branche dev/DS-936-update-react-data-table mais je crois celui ci est un fork de ca. Wait je vais essayer de fix ca.

@savutsang savutsang reopened this Jan 25, 2024
@savutsang savutsang changed the base branch from dev/DS-936-update-react-data-table to dev/DS-936-update-react-table January 25, 2024 19:33
@savutsang
Copy link
Contributor

savutsang commented Jan 25, 2024

  • J'ai change ton target pour ma branche actuel a place
  • J'ai mis a jour ta branche avec la derniere version, il etait un peu loin et avait un couple de conflicts.

Tout est beau now, desole pour le trouble.

@@ -76,9 +76,16 @@ function getHeading<TData extends object, TValue>(
sortState = 'descending';
}

if (!header.column.columnDef.header && !header.column.columnDef.headerAriaLabel) {
console.warn(
`headerAriaLabel missing for column ${header.id}. aria label is required for headers without text.`,
Copy link
Contributor

@savutsang savutsang Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si tu veux mentionner l'attribute: aria-label
Si tu veux mentionner la conformite: ARIA label (ARIA etant un accronyme, ca s'ecrit toujours en majuscule).

@alexbrillant alexbrillant merged commit 1100d24 into dev/DS-936-update-react-table Feb 2, 2024
20 checks passed
@alexbrillant alexbrillant deleted the dev/ACC-930 branch February 2, 2024 21:16
savutsang added a commit that referenced this pull request Feb 14, 2024
* fix(deps): update dependency react-table

* fix: update export

* fix: create file for table footer

* fix: create file for table header

* fix: update rows with new implementation

* fix: update table with new table version

* fix: update stories with new version

* fix: update tests

* fix: removed unused files

* fix: typing and formatting

* fix: formatting problem

* fix: sticky footer not working

* fix: sortable headers has wrong styles

* fix: lint and tests

* fix: initial sorting

* fix: compatible SC6

* fix: remove defaultSort from column

* chore: add table optimization example

* chore: update storybook description

* chore: doc

* chore: simplidy table example code

* fix: sc6 props, clean typings, various fixes

* fix: snapshot

* feat(Table): add headerAriaLabel prop for header columns (#696)

* feat(Table): add headerAriaLabel prop

* feat(Table): add aria label on header column def type

* fix: tests

* feat(Table): add aria-label warning for headers without text

* feat(Table): fix header aria-label warning

* feat(Table): fix eslint in table-header

* feat(Table): use warn instead of error for aria label warning

* feat(Table): fix storybook for header aria label

---------

Co-authored-by: Savut Sang <[email protected]>

* fix: remove extra scrollbars in stories

* fix: remove extra background in snapshots

* feat(Table): manual sorting (#719)

* feat(Table): manual sorting

* fix: comments

* fix: rename props

* fix: use max-height in stories

---------

Co-authored-by: Carla Franca <[email protected]>
Co-authored-by: Alexandre Brillant <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants