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

fix(Listbox): add bgcolor to selected indicator #1033

Merged
merged 3 commits into from
Dec 9, 2024
Merged

Conversation

LarryMatte
Copy link
Contributor

@LarryMatte LarryMatte commented Nov 14, 2024

DS-1268
Il manquait la couleur background sur l'indicateur de l'option sélectionnée.

Avant:
Screenshot 2024-11-14 at 11 03 23 AM

Après:
Screenshot 2024-11-14 at 11 03 48 AM

@LarryMatte LarryMatte requested a review from a team as a code owner November 14, 2024 16:02
Copy link

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

Copy link

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

maboilard
maboilard previously approved these changes Nov 29, 2024
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 questionnement, sinon LGTM.

@@ -179,6 +179,7 @@ const ListItem = styled.li<ListItemProps>`

${({ $selected, $multiselect }) => (!$multiselect && $selected && css`
&::before {
background-color: ${({ theme }) => theme.component['listbox-item-indicator-selected-background-color']};
Copy link
Contributor

Choose a reason for hiding this comment

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

Je pense que le nom de token est pas bon. Ça devrait peut-être être juste listbox-item-selected-indicator-color ou listbox-item-indicator-selected-color. Le terme background-color est trop lié à comment c'est implémenté et non ce que ça représente je crois.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maboilard Voir le commentaire de Max

Choose a reason for hiding this comment

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

Pas de problème pour moi. Si c'est plus logique comme ça, allons-y avec listbox-item-indicator-selected-color

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maboilard parfait assures-toi de faire la modification dans figma

@LarryMatte LarryMatte merged commit 4f436be into master Dec 9, 2024
22 checks passed
@LarryMatte LarryMatte deleted the dev/DS-1268 branch December 9, 2024 14:17
LarryMatte added a commit that referenced this pull request Jan 9, 2025
* fix(Listbox): add bgcolor to  selected indicator

* fix(Listbox): fix lint

* fix(Listbox): fix token name
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.

4 participants