-
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(Modal): adjust styling, adds width flexibility and header is now bold #984
Conversation
Storybook for this build: https://ds.equisoft.io/pr-984/ |
Webapp for this build: https://ds.equisoft.io/pr-984/webapp/ |
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.
Petit commentaire à valider.
Sinon en ce qui concerne la tâche, c'est good pour moi. Je laisse les devs faire le tour pour les modifications au code.
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.
Une autre passe.
Quelques commentaires/suggestions, à toi de voir.
gap: var(--spacing-2x); | ||
max-height: 100%; | ||
overflow-y: auto; | ||
padding: ${getHeightPadding} ${getWidthPadding}; |
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 me demande de la pertinence du bout de code où nous venons spécifier s'il y a un header
et un footer
.
Si nous enlevons le padding du <header>
<main>
et <footer>
et l'ajoutons seulement sur le container de la modal, je crois que nous n'aurions pas besoin de tout ça et nous sauverions quelques lignes de CSS.
Le padding
de la modal va empêcher l'élément <main>
d'être collé sur le haut et le bas de la modal s'il n'y a pas de header
et footer
.
Pour l'espacement entre le header
le main
et le footer
,
Il peut y avoir un margin-bottom
sur le heading (h2
) et un margin-top
sur le footer.
- Le
margin-bottom
du heading reste celui spécifier sur les headings du DS et il va permettre de pousser lemain
. S'il n'y a pas deheader
, lemain
va être collé aupadding
top de la modal. - Même principe pour le
footer
, nous pouvons lui ajouter unmargin-top
qui va créer l'espace avec lemain
. S'il n'y a pas defooter
, lemain
va être collé aupadding
du bas de la modal.
Ça va p-e simplifier aussi le no-padding car il va seulement être sur le container et non sur tous les éléments (header
, main
, footer
)
Nous n'aurions pas besoin non plus du gap
dans la section <main>
. Les éléments de textes vont garder le même espacement que dans le reste de l'app sans avoir à overwrite leurs styles.
J'ai fait un exemple sur codepen. Peut-être que j'oublie certaines choses, mais j'ai l'impression que ca simplifie pas mal et nous pourrions seulement utiliser du CSS au lieu d'ajouter des conditions selon la présence ou non d'un header
/footer
.
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.
ah nvm, je croyais que c'était un improvement que tu avais fait. Je viens de voir que c'était déjà comme ça. Stress pas pour ça, nous y reviendrons plus tard.
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 laisse les devs juger du code mais en ce qui me concerne, c'est good.
DS-1196
Description
Ici on vient;
width
permettant de modifier de la modalebold
)Tests fonctionnels