-
Notifications
You must be signed in to change notification settings - Fork 57
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: update Tile component #150 #277
Conversation
Merci beaucoup! |
src/Tile.tsx
Outdated
@@ -63,15 +113,29 @@ export const Tile = memo( | |||
"explicitlyProvidedId": id_props | |||
}); | |||
|
|||
if (!!disabled && linkProps !== undefined) { | |||
linkProps.href = undefined; |
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.
Plutôt que de muter le linkProps, je pense que c'est plus claire de faire L151 :
<Link
{...linkProps}
href={disabled ? undefined : linkProps.href}
className={cx(classes.link, linkProps.className)}
aria-disabled={disabled}
>
{title}
</Link>
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.
Fait. J'avais pas pensé à cette façon, mais je suis d'accord ça fait plus propre
Je ne suis pas favorable à une version majeure étant donné que le DSFR n’a pas fait de mise à jour majeure. Cependant, cela reste un changement important (breaking change). Le DSFR a gardé l’ancienne version en mode déprécié, ce qui est plus difficile pour nous, mais cela peut être une option : conserver l’ancienne version en la renommant. Cela reste un changement important, mais les utilisateurs de l’ancienne version de Tile n’auraient pas grand-chose à modifier en mettant à jour leur version s’ils souhaitent conserver leurs Tiles. |
@garronej Je te laisse bump la version 😎 |
Oups, j'avais laissé un nom de variable temporaire, du coup j'ai recréé une PR #278 |
❗BREAKING
#150