Skip to content

Conversation

@mtlmatt
Copy link
Collaborator

@mtlmatt mtlmatt commented Aug 2, 2023

Changes to accomodate a medium scrollable modal. The size was not limited to the window height

@mtlmatt mtlmatt force-pushed the feature/modal-sizing branch from 476f22d to edfaaed Compare August 2, 2023 15:05
@mtlmatt mtlmatt force-pushed the feature/modal-sizing branch 2 times, most recently from db119ab to 39160f9 Compare August 2, 2023 15:39
@OlivierAlbertini OlivierAlbertini requested a review from a team August 2, 2023 20:32
overflow: auto;

// The dialog container should completely fill its parent overlay element.
width: 100%;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Peux tu ajouter un test qui permet de démontrer que si je mets 2 contenus (de tailles différentes), la modale aura une taille différente ?

C'est pour s'assurer que width/height:100% n'est pas récupéré par un parent (ou un possible parent lors des prochains commits) afin d'éviter des regressions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: peut être que le test ne devrait pas être sur le contenu de la modale mais en rapport avec l'element parent.

@OlivierAlbertini
Copy link
Member

@gregpetit j'aimerais juste valider que le comportement est ok avec toi. On adapte la taille de la modale en fonction du contenu si j'ai bien compris le sens de la PR.

@OlivierAlbertini
Copy link
Member

Changes to accomodate a medium scrollable modal. The size was not limited to the window height

le 100% est enlevé sur le container donc j'imagine que ça n'impacte pas seulement le medium, mais pour toutes les tailles de la modale, non ?

@mtlmatt
Copy link
Collaborator Author

mtlmatt commented Aug 3, 2023

le 100% est enlevé sur le container donc j'imagine que ça n'impacte pas seulement le medium, mais pour toutes les tailles de la modale, non ?

Exact, le comportement de la modal small etait similaire et pouvait depasser la taille maximale de la fenetre

@gregpetit gregpetit self-requested a review August 3, 2023 13:38
Copy link
Contributor

@gregpetit gregpetit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtlmatt @OlivierAlbertini Tout est beau.
La hauteur de la modale s'ajuste bien et le défilement fonctionnement bien lorsqu'on atteint la hauteur de l'écran.

@mtlmatt mtlmatt force-pushed the feature/modal-sizing branch from 39160f9 to 2e9ea71 Compare August 3, 2023 15:13
@mtlmatt mtlmatt force-pushed the feature/modal-sizing branch from 2e9ea71 to ff7cbfd Compare August 3, 2023 15:59
Signed-off-by: Matthieu <matthieu.perrin@montreal.ca>
@mtlmatt mtlmatt force-pushed the feature/modal-sizing branch from ff7cbfd to e27180a Compare August 3, 2023 16:53
Copy link
Member

@OlivierAlbertini OlivierAlbertini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@OlivierAlbertini OlivierAlbertini added the bug 🔥 Something isn't working label Aug 3, 2023
@mtlmatt mtlmatt removed the request for review from TolaSam August 3, 2023 19:38
@mtlmatt mtlmatt merged commit 7328bfd into VilledeMontreal:master Aug 3, 2023
mtlmatt added a commit that referenced this pull request Aug 4, 2023
Signed-off-by: Matthieu <matthieu.perrin@montreal.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug 🔥 Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants