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.

Sign up for GitHub

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

Jump to bottom

feat(SegmentedControl)!: new Style and add icon #995

Merged
merged 10 commits into from
Sep 30, 2024
Merged

feat(SegmentedControl)!: new Style and add icon #995

merged 10 commits into from
Sep 30, 2024

Conversation

LarryMatte
Copy link
Contributor

DS-1231

  • Modifier le style du composant
  • Ajout option d'avoir des icônes ou non
  • Ajout option d'avoir tjrs un bouton de pressed
  • Modification dans les tokens du composant (Breaking change)

@LarryMatte LarryMatte requested a review from a team as a code owner September 19, 2024 14:15
@github-actions GitHub Actions
Copy link

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

@github-actions GitHub Actions
Copy link

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

savutsang
savutsang previously approved these changes Sep 20, 2024
View reviewed changes
maboilard
maboilard requested changes Sep 23, 2024
View reviewed changes
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.

Quelques commentaires :

  • Le border-radius de l'état hover semble être de 8px mais dans Figma il est de 4px
  • Dans l'état hover, le padding-left et right devrait être de 4px et identique au padding-top et bottom
  • Le letter-spacing du texte lorsqu'une option est pressed semble beaucoup plus prononcé que ce qu'il y a dans Figma

maboilard
maboilard previously approved these changes Sep 25, 2024
View reviewed changes
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.

LGTM!

WilliamsTardif
WilliamsTardif previously approved these changes Sep 26, 2024
View reviewed changes
meriouma
meriouma reviewed Sep 30, 2024
View reviewed changes
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.

LGTM

packages/react/src/components/segmented-control/segmented-control.tsx Outdated Show resolved Hide resolved
packages/react/src/components/segmented-control/segmented-control.tsx

return (
<Container className={className} role="group" aria-label={groupName}>
{buttonGroup.map((button, i) => (
<ToggleButton
aria-label={button.label}
aria-label={button.ariaLabel || undefined}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ça devrait pas faire un fallback vers button.label?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dans le cas où il y a seulement une icône, il n'y aura pas de label

Copy link
Contributor

@meriouma meriouma Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Mais dans le cas où il y a un button.label, mais pas de button.ariaLabel, est-ce qu'on veut quand même un aria-label avec button.label comme valeur? Ou on laisse la techno gérer avec le label qui va être en texte dans le span?

Copy link
Contributor Author

@LarryMatte LarryMatte Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

est-ce qu'on veut quand même un aria-label avec button.label comme valeur?

C'était semi comme ça avant mais je ne vois pas ce que ça apporte de plus de passer le label par l'aria-label sachant que le texte est déjà présent dans le span. Pour l'a11y il n'y a pas de gain et ça ajoute des éléments qui ne sont pas nécessaire dans la DOM. Est-ce que tu y vois un avantage?

packages/react/src/components/segmented-control/segmented-control.tsx Show resolved Hide resolved
packages/react/src/themes/tokens/component/segmented-control-tokens.ts Outdated Show resolved Hide resolved
@LarryMatte LarryMatte dismissed stale reviews from WilliamsTardif and maboilard via d092316 September 30, 2024 13:34
pylafleur
pylafleur approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@pylafleur pylafleur left a comment

Choose a reason for hiding this comment

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

LGTM!

@LarryMatte LarryMatte merged commit 0a78376 into master Sep 30, 2024
22 checks passed
@LarryMatte LarryMatte deleted the dev/DS-1231 branch September 30, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meriouma meriouma meriouma left review comments

@pylafleur pylafleur pylafleur approved these changes

@maboilard maboilard maboilard left review comments

@WilliamsTardif WilliamsTardif WilliamsTardif left review comments

@savutsang savutsang Awaiting requested review from savutsang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@LarryMatte @meriouma @pylafleur @savutsang @WilliamsTardif @maboilard