-
Notifications
You must be signed in to change notification settings - Fork 0
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(Slider): add new slider component #893
base: master
Are you sure you want to change the base?
Conversation
Storybook for this build: https://ds.equisoft.io/pr-893/ |
Webapp for this build: https://ds.equisoft.io/pr-893/webapp/ |
'slider-rail-background-color': 'color-bg-empty', | ||
'slider-rail-disabled-background-color': 'color-bg-neutral-subtle', | ||
'slider-rail-mark-background-color': 'color-bg-neutral-bold', | ||
'slider-rail-mark-disabled-background-color': 'color-neutral-30', // color-background-indicator-disabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beaucoup d'alias sont creer ou renomme dans ce PR. A re-ajuster une fois qu'il est merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Il manquerait le tooltip lorsque l'utilisateur interagit avec le composant en utilisant les touches (flèches) du clavier. Présentement le tooltip est seulement affiché lorsqu'un user utilise la souris. MUI semble avoir la feature.
-
Présentement, il n'y a pas de label associé à l'input et ça vient créer une failure au niveau de l'a11y. La valeur de l'attribut
for
dulabel
pointe vers l'id
du<span>
qui englobe le slider au complet. Je ne sais pas à qu'elle point nous avons le contrôle là dessus, mais il faudrait qu'il soit associé à l'id
de l'input type="range"
. Sinon, en regardant ce que MUI ont, ils semblent avoir unaria-label
sur les thumbs. Ça pourrait également venir fixer le problème.
Sinon un petit commentaire en lien avec une couleur.
|
||
&::before { | ||
background-color: ${({ theme }) => theme.component['slider-thumb-background-color']}; | ||
border: 2px solid ${({ theme }) => theme.component['slider-thumb-border-color']}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'ai l'impression que la couleur n'est pas la bonne ici.
rgb(0, 58, 90)
vs. rgba(0, 98, 150, 1)
/ #006296
dans figma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool POC avec mui. LGTM pour le code. Je laisse Larry et Marc regarder le reste.
font-size: 0.75rem; | ||
padding: var(--spacing-half) var(--spacing-1x); | ||
position: absolute; | ||
top: 41px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C'est pas lié à la PR nécessairement, mais je me demande, ce serait pas mieux si on avait tout en rem
pour le zoom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'ai zoomé la page au complet, changé la taille du texte, et ca ne semble pas changer grand chose.
Par contre, je suis un peu du même avis de changer le 41px
pour 2.5rem
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quelques petits commentaires:
- Le font-size et le line-height du label et des numéros sont trop grands
- La couleur des numéros n'est pas la bonne. Le token devrait référer sur le alias token "color-content-subtle"
- La couleur de border du Slider-thumb n'est pas la même que sur les maquettes Figma
DS-281
Description