-
Notifications
You must be signed in to change notification settings - Fork 33
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
Refactor Button Component with New Variants and Props #772
base: slash-releases/2.0.0
Are you sure you want to change the base?
Conversation
bf061fe
to
bace0b6
Compare
6b3538d
to
16d1064
Compare
16d1064
to
482485e
Compare
482485e
to
e4d1851
Compare
Lorsqu'il y a des breaking change impliqué par une PR, il faudrait que cette PR mette à jour tout de suite le guide de Migration, ca évitera de le faire au dernier moment avant la livraison de la nouvelle version. Je vois qu'ici il n'y a que le guide de migration depuis le premier toolkit. Le MIGRATION.md peut être initialisé grâce à cette PR. |
Yes pas faux j'ai complément zapper le guide de migration, je le fais des que possible |
e4d1851
to
267af16
Compare
|
267af16
to
6ddfa45
Compare
53747d6
to
79407ee
Compare
@mixin label { | ||
font-family: var(--font-family-base); | ||
font-size: var(--font-size-16); | ||
font-weight: var(--font-weight-bold); | ||
line-height: var(--font-line-height-20); | ||
} |
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.
je ne vois pas l'intéret de cette mixin. Passons nous en !
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.
le est d'avoir des mixin utilitaire que corresponde au fonts style du DS https://zeroheight.com/4b1e27a45/v/latest/p/9506de-fonts-style
Afin que si cela change on est à le changé qu'a un seul endroit, maintenant vue qu'on c'est dit de minimiser l'utilisation de sass je pense qu'il faudrait transformer la mixin en class css utilitaire.
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.
79407ee
to
50ea1cb
Compare
slash/react/src/Button/Button.tsx
Outdated
import "@axa-fr/design-system-slash-css/dist/Button/Button.scss"; | ||
|
||
import { ComponentPropsWithoutRef, PropsWithChildren } from "react"; | ||
import { getComponentClassName } from "../utilities"; | ||
export type Variant = "primary" | "secondary" | "validated" | "danger"; |
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.
question: Est ce que c'est utile de mettre "primary" dans les variant
s ?
Ca peut éviter de mettre une condition sur === "primary"
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.
Je vois ce que tu veux dire, ou alors on veut faire un truc à la bootstrat a savoir ajouter une classe css af-btn--primary
(d'ailleurs idem pour le size) cela évitera la condition on aurait juste classNames(DEFAULT_CLASS_NAME,
${DEFAULT_CLASS_NAME}--${variant},
${DEFAULT_CLASS_NAME}--${size}, className)
Parce que perso je ne suis pas fan du fait de ne pas mettre le variant primary. mais c'est juste une pref perso.
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 fais des changements j'ai pris exemple sur le fonctionnement de bootstrap ou il faut mettre les class af-btn
et af-btn--primary
au minimum.
et j'ai passer la props size en small
en mode boolean.
dit moi ce que tu en penses.
50ea1cb
to
b487419
Compare
BREAKING CHANGE: **button**, removed universes variant; use custom class if needed BREAKING CHANGE: **button**, removed hasIconLeft/hasIconRight; CSS auto-detects use of SVG or glyphicon BREAKING CHANGE: **button**, variant reverse is now renamed to secondary BREAKING CHANGE: **button**, variant success is now renamed to validated BREAKING CHANGE: **button**, removed disabled variant; now use button's disabled prop BREAKING CHANGE: **button**, removed circle variant
…on button component BREAKING CHANGE: **button**, prop `className` no longer removes default style BREAKING CHANGE: **button**, prop `classModifier` is removed; use props `variant`, `size`, `leftIcon`, and `rightIcon` instead
b487419
to
48d3299
Compare
Description:
This PR introduces significant changes to the button component, including a restyle with the new-look Slash DS and the introduction of variant props instead of classModifier. The following breaking changes are included
closes #751
Breaking Changes:
Restyle Button with New-Look Slash DS
universes
variant; use custom class if needed.hasIconLeft
/hasIconRight
; CSS now auto-detects use of SVG or glyphicon.reverse
variant tosecondary
.success
variant tovalidated
.disabled
variant; now use button'sdisabled
prop.circle
variant.Introduction of Variant Props
className
no longer removes default style.classModifier
; use propsvariant
,size
,leftIcon
, andrightIcon
instead.Additional Information:
For the CSS part, I have introduced the use of CSS variables for colors, fonts, etc., as well as mixins to provide various shortcuts for creating our CSS classes. All these modifications are open to discussion and may lead to the drafting of an ADR on CSS best practices.
Screanshot: