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(dropdown-list): DS-1126: add icon inside the Dropdown control #873

Merged
merged 1 commit into from
May 27, 2024

Conversation

WilliamsTardif
Copy link
Contributor

@WilliamsTardif WilliamsTardif commented May 9, 2024

Copy link

github-actions bot commented May 9, 2024

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

Copy link

github-actions bot commented May 9, 2024

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

@WilliamsTardif WilliamsTardif changed the title feat(dropdown-list, storybook): DS-1126: add icon in front of the options of dropdown-list feat(dropdown-list, storybook): DS-1126: add icon iinside the Dropdown control May 10, 2024
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.

Il manque le aria-hidden="true" sur l'icône.

Si je superpose ce que tu as et ce qui est dans Figma, il y a un petit décalage. Ton icône est trop vers la droite comparé au visuel. Tu peux utiliser cette extension chrome: https://chromewebstore.google.com/detail/perfectpixel-by-welldonec/dkaagdgjmgdmbnecmcefdhjekcoceebi
Screenshot 2024-05-10 at 8 50 19 AM

Tu vas pouvoir faire une capture d'écran (zoom à 100%) de ce qui est dans Figma et ensuite l'importer par dessus ce sur quoi tu travailles dans chrome. Ça va te permettre d'être le plus 'pixel perfect' possible. Pokes moi si tu veux que je te montre comment tout ca fonctionne.

@savutsang
Copy link
Contributor

Typo dans le title du PR iinside

Aussi IMHO, on ne devrait pas mettre "storybook" dans le scope lorsque c'est un feat ou fix. Ce n'est pas utile au point de le mentionner, ca fait juste polluer le git history. Je prefere mettre de l'emphase sur le feature principal feat(dropdown-list)

@WilliamsTardif WilliamsTardif changed the title feat(dropdown-list, storybook): DS-1126: add icon iinside the Dropdown control feat(dropdown-list): DS-1126: add icon iinside the Dropdown control May 13, 2024
@WilliamsTardif WilliamsTardif changed the title feat(dropdown-list): DS-1126: add icon iinside the Dropdown control feat(dropdown-list): DS-1126: add icon inside the Dropdown control May 13, 2024
@@ -112,6 +112,10 @@ const Arrow = styled(Icon)<{ $disabled?: boolean }>`
width: var(--size-1x);
`;

const TextIcon = styled(Icon).attrs({ color: '#60666E' })`
Copy link
Contributor

Choose a reason for hiding this comment

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

Dernier points, faudrait utiliser le theming. Je pense il faudrait ajouter un token dans dropdown-list-tokens.ts, genre

'dropdown-list-input-icon-color': 'color-neutral-65'

@maboilard?

Y a 2 facons d'utiliser ca, la premiere est plus clean avec un prop bien controlle. Mais honnetement, l'un ou l'autre c'est pas un truc qui me gosse.

Choose a reason for hiding this comment

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

En fait, ça donnerait ceci:
'dropdown-list-input-icon-color': 'color-content-subtle'
@hebernardEquisoft Est-ce que c'est quelque chose qui devrait être fait ici ou dans ta PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ça peut se faire ici sans problème! les alias-tokens existent maintenant sur master! 💯

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.

Bien parfait pour moi en ce qui me concerne.
Je laisse Savut, Max et PY être les juges pour le restant.

@savutsang
Copy link
Contributor

Il reste juste a changer la couleur pour 'color-content-subtle' (#873 (comment)) pis on est bon, le reste est LGTM.

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.

J'ai fait le tour et ça semble all good pour moi!

Copy link
Contributor

@alexbrillant alexbrillant left a comment

Choose a reason for hiding this comment

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

🍆

@WilliamsTardif WilliamsTardif merged commit f651bff into master May 27, 2024
22 checks passed
@WilliamsTardif WilliamsTardif deleted the dev/DS-1126 branch May 27, 2024 20:44
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.

6 participants