-
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 Footer Component and restyle with the new-look Slash DS #838
base: slash-releases/2.0.0
Are you sure you want to change the base?
Conversation
499282d
to
1af2ca7
Compare
d7a29d9
to
0394b6a
Compare
Je pense que cette PR mélange énormément de sujet très structurant, pour la faire avancer le plus simple serait de séparer les sujets. Par exemple pour l'ajustement visuel du footer, ca pourrait partir dans une v1 si seulement il y avait que ça. La grid ou les autres sujets pourraient être tirer à part, quand ils auront été validés en équipe |
@@ -0,0 +1,37 @@ | |||
import { |
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.
ça serait bien de séparer le système de Grid dans une autre PR pour réduire la complexité de celle-ci.
De plus, @samuel-gomez a déjà mis en place une Grid sur L&F, pourquoi en redévelopper une nouvelle plutôt que capitaliser sur celle-là ?
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 peux effectivement séparer la grid dans une autre PR, mais j'en aurais besoin pour cette PR.
Pour ce qui est de la grid slash ce c'est pas la même que celle de L&F. dans notre cas nous avons qu'une seul collonne.
https://zeroheight.com/4b1e27a45/v/latest/p/5045af-breakpoints
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.
on n'a pas qu'une seule colonne actuellement sur slash étant donné qu'on utilise bootstrap. Les breakpoints n'influent pas sur la grille que l'on a ensuite derrière
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.
Alors je ne remet pas en cause l'utilisation de la grid bootstrap, cependant quand je lit les règles du DS sur zeroheight il y en a aucun mentions, alors il y a 2 solutions, sois c'est un oublie, auquel cas les UX/UI devrait corriger la doc zeroheight, sois c'est effectivement le cas et on aura plus besoin de la grid bootstrap, d'ailleurs il prévue de ce séparer de la grid bootstrap mais on a pas de remplaçant actuellement.
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.
La grid bootstrap va disparaitre c'est une certitude, mais ce n'est pas encore pour tout de suite. La suppression de bootstrap est prévue en v3 dans la roadmap, mais en attendant on doit la conserver et au sein de la grid on doit bien continuer à avoir les 12 colonnes (en mode full) comme bootstrap pour le moment.
La Grid que @samuel-gomez a implémenté sur L&F permet cela et il me semble qu'on avait bien dit qu'elle serait repris ensuite sur Slash. @JLou tu confirmes ?
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.
La suppression de la grille c'est prévu pour la v3, là on serait vraiment trop en avance de phase je pense.
On peut pas juste garder la grille actuelle et "corriger" les marges et taille pour correspondre à ce qu'attend le zero height ?
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.
Ben justement je n'ai pas supprimé la grid bootstrap elle est toujours la et peut être utiliser au besoin.
Et je le re dit si slash dois avoir une grid en 12 colonnes cela doit être indiqué dans les règles du DS sur zeroheight.
Si ca n'est pas présent sur zeroheight cela n'a donc rien à faire dans ce toolkit.
- <Footer title="Axa"> | ||
- <strong>@ {new Date().getFullYear()} AXA</strong> | ||
- </Footer> | ||
+ <Footer version="1.0.0" /> |
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.
que se passe-t-il si l'on veut mettre autre chose qu'une version dans le footer ? ne plus avoir de children bloque complètement l'utilisation du composant je trouve
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.
Justement vue les règles du design system on ne peut rien mettre d'autre dans le footer que le logo, le copyright et la version
https://zeroheight.com/4b1e27a45/v/latest/p/64fb51-footer/b/15c512/t/f7f70e223c
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 peur que ça ferme le composant et qu'on se retrouve possiblement bloqué là-dessus. Qu'en pensez-vous @JLou @samuel-gomez @johnmeunier ?
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'en ai parler parler avec @adrien-dos (notre UX sur slash) est il ma dit que le footer b2b ne devrait contenir rien autre.
Si c'est pas le cas il faut l'indiqué dans les règles du DS sur zeroheight.
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 suis partagé, j'avais failli faire la remarque.
Effectivement le zero height ne parle pas de children. Après j'aime pas bloquer le consommateur, ici juste donner la possibilité de débrancher le footer et ajouter un children "au cas où" l'UX sur le projet consommateur demanderait une spécificité permettrait au consommateur de toujours utiliser le composant footer et répondre au besoin. Un bon compromis je pense
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 suis d'accord, j'ai déjà eu le coup sur des composants côté L&F où j'avais trop restreint en me basant sur la maquette et sur les règles théoriques. Au final on a eu un cas exceptionnel sur notre projet où on avait besoin de modifier un petit truc et on était bloqué.
Au moins avec une valeur par défaut qu'on peut remplacer si besoin ça permet une certaine flexibilité tout en gardant le comportement voulu de base.
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.
Oui c'est un peu l'une des préco de mettre des ReactNode plutôt que des string pour avoir un peu plus de souplesse sur nos composants
@@ -0,0 +1,36 @@ | |||
@use "sass:map"; |
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.
dans le fichier common je vois qu'on a déjà beaucoup de code qui s'apparentent à des breakpoint, est-ce qu'il ne faudrait pas tout mettre au même endroit ?
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.
Justement c'est ce but de ce fichier, sauf que au lieu de tous séparer je déplace/place ce dont j'ai besoin afin qu'a therme la seul chose qui reste du common parte à la corbeille.
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.
pourquoi ne pas supprimer du common également du coup ?
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.
parce que je ne sais pas se qui est utilisé ou pas actuellement dans notre toolkit slash.
Je préfère le faire à la fin.
@@ -0,0 +1,51 @@ | |||
/** @type { import('@storybook/addon-viewport').ViewportMap } */ |
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 le même fichier que dans le package css ? ne peut-on pas le mettre dans un dossier commun ?
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 tenté mais cela ne pas marché si tu as une solution je suis preneur
ref={ref} | ||
> | ||
{/* eslint-disable-next-line jsx-a11y/alt-text */} | ||
<img className="af-footer__logo" src={logoAxa} aria-hidden /> |
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.
est-ce que l'img avec role="presentation"
n'est pas mieux qu'en aria-hidden ?
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.
justement d'après le RGAA https://accessibilite.numerique.gouv.fr/methode/criteres-et-tests/#1.2
il faut soit un aria-hidden
ou un role="presentation"
, perso je trouve que le aria-hidden
est plus parlant
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'utilise plutôt role="presentation" de mon côté.
@JLou @samuel-gomez @johnmeunier qu'en pensez-vous ?
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.
Perso même si dans l'absolu role="presentation"
indique clairement que l'image n'a pas un rôle d'image, on ne voit pas de suite les implications induite.
Alors qu'avec un aria-hidden
, notre image garde sont role="image"
donc on peut facilement la capture lors des tests, mais surtout l'attribut aria fait directement référence à l'accessibilité et donc ici masque l'image pour les lecteurs d'écran.
Mais dans l'absolu les 2 solutions sont valide RGAA donc je me rangerai à l'avis majoritaire.
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.
Franchement pas d'avis pour le coup, mais un bon point à remonter dans les guidelines ! J'aurais presque envie de dire de mettre les deux pour être explicite.
Par contre je vois pas dans quel cas on aurait besoin de tester une image qui serait en hidden ? Un test sur quelque chose que tout le monde n'aurait pas accès ? Je vois pas d'exemple
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.
Si c'est juste pour mettre en décorative l'image, un simple alt vide devrait suffire pour les lecteurs d'écran non ?
https://www.w3.org/WAI/tutorials/images/decorative/
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 une des 3 méthodes que le RGAA recommande https://accessibilite.numerique.gouv.fr/methode/criteres-et-tests/#1.2.1
Il ne faut pas oublié que nous devons respecté en France le RGAA qui est une extension du WCAG
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.
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.
yes après je pense entre aria-hidden
ou role="presentation"
c'est plus une histoire de gout.
Je suis d'accord que le sujet de grid devrait être dans une PR à part car elle n'a aucun rapport avec le Footer. |
Pour moi je peux séparer la grid dans une PR mais dans tous les cas j'en aurais besoin pour le footer, par contre le footer étant tellement différent d'un point de vue des règles DS que je ne peux pas le modifier sans break donc pour la v2 |
Oui bien sûr, on peut faire comme ça du coup 👍 |
Qu'est-ce qui diffère exactement sur le footer qui force à break ? |
le footer n'a plus la même structure html et donc les même class css ce qui implique que les consommateurs aurons du travaille s'il cible les class css. donc tous ca fait que ca break. |
Là on parle de contrainte technique en préparant le futur. Mais fonctionnellement on parle que de l'ajout de la version en prop et des tailles / marges, donc on pourrait juste faire ces modifs avec le composant actuel sans break et l'envoyer dans la v1 nan ? |
a0fad3d
to
ba7d205
Compare
Conformément à nos discussions j'ai séparer la grid dans une PR dédier ici #892 cette PR seras en attente le temps que l'autre PR sois merge et rebase sur la v2. |
BREAKING CHANGE: **footer**, props `className` don't remove default class BREAKING CHANGE: **footer**, new props `version` for set the version of application BREAKING CHANGE: **footer**, new structure for footer based on display grid
ba7d205
to
e32a641
Compare
Description:
This PR introduces significant changes to the Footer component, including a restyle with the new-look Slash DS and new simple html structure. The following breaking changes are included
closes #809
add change
Breaking Change
Screenshot: