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(tabs)!: add delete tabs #747

Merged
merged 23 commits into from
May 1, 2024
Merged

feat(tabs)!: add delete tabs #747

merged 23 commits into from
May 1, 2024

Conversation

savutsang
Copy link
Contributor

@savutsang savutsang commented Feb 26, 2024

DS-764

Changes

  • Added a feature to allow to add and remove tabs.
  • New modern design for the Tabs.
  • Rework on the design tokens to reflect the new design.

Others

  • Improved the test utility findByTestId/getByTestId to be able to target more precisely the wanted node (ex: mount() wrapper might create 2-3 nodes with the same data-testid).

BREAKING CHANGE:

  • Removed "contained" prop. The new design don't allow this customization anymore.

@savutsang savutsang requested a review from a team as a code owner February 26, 2024 15:21
Copy link

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

Copy link
Contributor

@LarryMatte LarryMatte left a comment

Choose a reason for hiding this comment

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

Quelques commentaires ici et là.

Sinon quelques modifications que j'apporterais au comportement clavier:

  • J'enlèverais la possibilité de focus (touche Tab du clavier) sur les "x" des tabs qui ne sont pas actives.
  • Dans la story "normal" quand j'utilise la touche Tab du clavier, le focus passe du Tab au TabPanel et après il devrait tomber sur le bouton de sorting dans le tableau mais il ne s'y rend pas. Comme si la navigation avec la touche Tab skippait ce qui se trouve dans le TabPanel.

Prochain point est mon erreur... my bad
lorsqu'un utilisateur ajoute plusieurs Tabs, les Tabs s'enlignent les unes à la suites des autres et elles deviennent toutes compressées. Dans Figma, il y a 2 boutons placés au début et à la fin du Tablist lorsque les Tabs dépassent l'espace prévu par le Tablist.
Il faudrait ajouter la features dans la PR. Toutes mes excuses. Il y a un exemple similaire dans le Design system de Carbon

packages/react/src/components/tabs/tabs.tsx Outdated Show resolved Hide resolved
packages/react/src/components/tabs/tab-button.tsx Outdated Show resolved Hide resolved
packages/react/src/components/tabs/tab-panel.tsx Outdated Show resolved Hide resolved
Copy link

@maboilard maboilard left a comment

Choose a reason for hiding this comment

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

Mes commentaires sont pas mal similares à ceux de Larry.

Il manquerait les boutons en cas d'overlflow (voir ici)

Aussi, le bouton ADD TABS semble pas mal différent du bouton tertiaire que l'on utilise habituellement. Serait-ce possible de réutiliser le bouton tertiaire afin que l'on ait l'effet hover, la forme ronde et tout les autres particularités que nos boutons ont?

C'est peut-être hors scope mais je vais faire le commentaire quand même. Dans le tablist, on a un padding-left de 32px mais celui-ci devrait être de 16px afin que le contenu de la section soit aligné avec le label du premier Tab. Voir sur l'exemple suivant le décalage:
image

Finalement, lorsqu'un Tab est sélectionné, il est toujours encerclé de son état focus. Est-ce que c'est vraiment nécessaire?

Je pense que ça fait pas mal le tour de mon côté

@savutsang
Copy link
Contributor Author

Finalement, lorsqu'un Tab est sélectionné, il est toujours encerclé de son état focus. Est-ce que c'est vraiment nécessaire?

Fixed

Copy link
Contributor

@meriouma meriouma left a comment

Choose a reason for hiding this comment

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

Petit review, je vais refaire une passe après les validations de Marc et/ou Larry.

packages/react/src/components/tabs/tab-button.tsx Outdated Show resolved Hide resolved
packages/react/src/components/tabs/tabs.tsx Outdated Show resolved Hide resolved
packages/react/src/i18n/translations.ts Show resolved Hide resolved
packages/react/src/components/tabs/tab-panel.tsx Outdated Show resolved Hide resolved
@savutsang
Copy link
Contributor Author

C'est peut-être hors scope mais je vais faire le commentaire quand même. Dans le tablist, on a un padding-left de 32px mais celui-ci devrait être de 16px afin que le contenu de la section soit aligné avec le label du premier Tab.

Fixed

maboilard
maboilard previously approved these changes Mar 15, 2024
Copy link

@maboilard maboilard left a comment

Choose a reason for hiding this comment

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

C'est comme si j'étais dans Figma! Excellent boulot Savut!! :)

@savutsang savutsang force-pushed the dev/DS-764 branch 2 times, most recently from e507bfd to 7f25032 Compare March 18, 2024 14:40
meriouma
meriouma previously approved these changes Apr 24, 2024
@savutsang savutsang enabled auto-merge (squash) April 24, 2024 17:27
@savutsang savutsang requested a review from LarryMatte April 24, 2024 17:28
@savutsang savutsang disabled auto-merge April 24, 2024 17:28
Copy link
Contributor

@LarryMatte LarryMatte left a comment

Choose a reason for hiding this comment

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

Quelques points:

  1. Quand le focus est sur le bouton “delete” et qu’on supprime la Tab ou qu’on supprime la Tab active avec la souris, le focus disparait et il n’y a plus de Tab active laissant ainsi un vide sous les tabs (il n'y a pas de tabpanel d'affiché i.e., aucun contenu sous les tabs) ET il n’est plus possible de focus sur les Tabs qui sont inactive. Il faudrait que le focus bouge vers la Tab suivante, la rendant active au même moment.
  2. Quand le focus est sur le bouton “delete” de la tab active, il faudrait pouvoir bouger à la prochaine Tab en utilisant la flèche du clavier (comme quand le focus est sur la tab elle même).
  3. Le aria-label du bouton pour supprimer une Tab n’a pas le label de la tab dedans, il y a seulement dismissTab. Il faudrait que ce soit Dismiss [nom du tab] Tab. Donc dismiss first button tab.

packages/react/src/components/tabs/tab-button.tsx Outdated Show resolved Hide resolved
Copy link

Webapp for this build: https://ds.equisoft.io/pr-747/webapp/

@savutsang
Copy link
Contributor Author

  • Quand le focus est sur le bouton “delete” et qu’on supprime la Tab ou qu’on supprime la Tab active avec la souris, le focus disparait et il n’y a plus de Tab active laissant ainsi un vide sous les tabs (il n'y a pas de tabpanel d'affiché i.e., aucun contenu sous les tabs) ET il n’est plus possible de focus sur les Tabs qui sont inactive. Il faudrait que le focus bouge vers la Tab suivante, la rendant active au même moment.

Fixed

  • Quand le focus est sur le bouton “delete” de la tab active, il faudrait pouvoir bouger à la prochaine Tab en utilisant la flèche du clavier (comme quand le focus est sur la tab elle même).

Fixed

  • Le aria-label du bouton pour supprimer une Tab n’a pas le label de la tab dedans, il y a seulement dismissTab. Il faudrait que ce soit Dismiss [nom du tab] Tab. Donc dismiss first button tab.

Fixed

@savutsang
Copy link
Contributor Author

Tout a ete fixe, pret pour un autre autre review.

@LarryMatte
Copy link
Contributor

LarryMatte commented Apr 29, 2024

  • Il faudrait que le focus bouge vers la Tab suivante, la rendant active au même moment.

@savutsang
Il reste à ajouter l'état focus i.e., le contour qui est highlighted, sur le Tab active après la suppression d'une Tab.

@savutsang
Copy link
Contributor Author

savutsang commented Apr 30, 2024

@savutsang Il reste à ajouter l'état focus i.e., le contour qui est highlighted, sur le Tab active après la suppression d'une Tab.

Fixed

@savutsang
Copy link
Contributor Author

Tout a ete fixe, pret pour un autre autre autre review.

Copy link
Contributor

@LarryMatte LarryMatte left a comment

Choose a reason for hiding this comment

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

LGTM
Good job!

@savutsang savutsang merged commit 2249458 into master May 1, 2024
23 checks passed
@savutsang savutsang deleted the dev/DS-764 branch May 1, 2024 15:56
pylafleur pushed a commit that referenced this pull request Jun 3, 2024
* feat(tabs): add delete tabs

* fix: panel border color

* fix: border-radius

* fix: comments

* fix: comments

* fix: comments

* fix: extract scrollable to a hook

* fix: post-reviewed ui

* fix: x focusable seulement le tab active

* fix: focus on panel

* fix: remove stylelint comment

* fix: simplify tests and testid

* fix: tabs focus inside

* fix: post review comments

* fix: better story for tabs

* fix: focus after delete, rename tokens, fix comments

* fix: comments
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.

5 participants