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): added align-top variation #3435
Conversation
Preview: https://patternfly-pr-3435.surge.sh CSS Size Report
A11y report: https://patternfly-pr-3435-coverage.surge.sh |
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.
This looks good to me @mcoker .
@ziccardi @garrett Can you review and confirm that this will address your original ask here: patternfly/patternfly-design#890
I don't know how I can check this in Cockpit or even see an isolated demo page... but when looking at the code and the screenshots, it looks like the dialog modification in this PR is stretched to the content area? We don't need all of our dialogs to be as large as possible. Although, I guess, these larger fullscreen dialogs might be useful for some — and perhaps even for us too, if we designed for it. (This might especially be the case if we had a way to toggle between normal and maximized mode, I guess?) What we definitely need: Dialogs that can be positioned relative to the top, so:
Context of the issues we're facing with the currently center-aligned dialogs in Cockpit @ patternfly/patternfly-design#890 (with screenshots) |
@garrett if my guess at the markup is correct, this is what the top-aligned modal in this PR will look like in a patternfly page where the content in the main section is an I just added the top-aligned example from https://patternfly-pr-3435.surge.sh/documentation/core/demos/modal/top-aligned as inline code for the iframe, with a |
@mcoker: Oh! That would look nice. I was thrown off by the (I wonder if there's a way to view branches of PF in Cockpit. Perhaps @KKoukiou or @martinpitt might know.) |
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.
LPTM 👍
LGTM 👍 |
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.
cc @mcarrano wdyt about christie's comment, should we offer a top variation with no margin |
I'm inclined to say no, @mcoker @christiemolloy . But what do you think, @mceledonia ? |
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.
lgtm! we can always add a variation for the modal to sit at the very top later
🎉 This PR is included in version 4.39.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Documentation - https://patternfly-pr-3435.surge.sh/documentation/core/components/modalbox
Demo - https://patternfly-pr-3435.surge.sh/documentation/core/demos/modal/top-aligned
fixes #3405
After looking at this some more, I think introducing a dialog layout would be too much hassle than it's worth at this point. We can just reposition the modal within the bullseye for now, and that should work fine. In an upcoming breaking change release we can re-evaluate and refactor the modal-box just to be a box and move all of the positioning, max-widths/heights, etc from it to the modal to a dialog layout.
I also evened out the outer spacing for this variation so that the top/bottom/left/right spacing around the modal is all the same.