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
add xxl size modal #39477
base: main
Are you sure you want to change the base?
add xxl size modal #39477
Conversation
Thanks for making this PR based on the issue I opened. :-) If it's approved we'd need to update the Modal docs too: |
In case this PR gets approved I went ahead and added the necessary documentation. |
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.
Thanks for creating this PR @TommasoAllegretti
Apart from the possible issue in scss/_modal.scss
, the code looks good to me!
However, we still to determine if we want this modal variant to be added to our build.
I'll let @twbs/css-review add their thoughts, but on my side, I wouldn't mind adding it since the CSS bundle wouldn't grow too much, and it seems consistent with what we provide for fullscreen modals, XXL containers, etc.
While we are deciding, I'm going to add the corresponding issue to the v5.4 project and set its state to "Review in Progress". Indeed, being a new feature, it couldn't be added before in a v5.3.x.
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.
The techinal part LGTM! Let's wait for the feedback of the rest of the team to see if this new feature can be embedded (in v5.4 or after) or not.
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.
I feel fine with the technical part as well and it feels right to have this kind of variant, we have those almost everywhere in the doc.
The only drawback I can see is kind of not related to this PR:
Should the lg, xl and xxl modals be "fluid" until they reach their maximum width ? The following image feels wrong to me. It could be done afterwards anyway.
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.
Works for me, as does @louismaximepiton's suggestion I think.
Description
add xxl modifier for modal
Motivation & Context
as requested in issue #39433
Type of changes
Checklist
npm run lint
)Live previews
Related issues
Closes #39433