feat(Page): remove extra margin on small viewports#7304
feat(Page): remove extra margin on small viewports#7304mcoker merged 2 commits intopatternfly:mainfrom logonoff:gh-7300
Conversation
|
Preview: https://patternfly-pr-7304.surge.sh A11y report: https://patternfly-pr-7304-a11y.surge.sh |
|
@lboehling how do you feel about removing the margins. I wonder if this should also reduce the padding on the page component? |
There was a problem hiding this comment.
I think this looks good overall!
one suggestion and one comment:
The sharp borders on the bottom of page content area look strange when the page body isn't filling the full height of the page. I wonder if on mobile we should default to/suggest having the page content area fill the height to avoid this.
wdyt @srambach @andrew-ronaldson ?
And I think the padding in the page component is fine for now. If we reduced it in the page body area on small viewports, we'd also want to reduce the page chrome padding on small viewports. We actually did this before in v5 but I remember it caused a lot of issues so we backed that decision out/decided to keep the padding consistent. @mcoker probably remembers more details on that!
|
@andrew-ronaldson @lboehling yeah any time we change the page section padding, we need to change the padding in some other components. Tables are a good example. If you remove page section padding around a table so the table row dividers span the width of the content area, since the page section padding and table padding are the same, the content aligns fine. 
If we change the page section padding from 24px to 16px, but don't change the table padding, you can see that the content isn't aligned: 
Changing the page section padding at a specific viewport just introduces some fragility because the padding of other components will likely also need to change, and it doesn't always make sense for the component padding to change between viewports based on whether the table, for example, is laid out as it is in my last screenshot above, or just a child of something in the page content area and doesn't span the width of the main content area. |
There was a problem hiding this comment.
Thanks for the PR @logonoff! I chatted with @lboehling and @andrew-ronaldson about this and we wanted to make a few changes below the md breakpoint:
- The
__main-containerelement should fill the viewport height below the md breakpoint (you should be able to set it toalign-self: stretch) - Remove all border-radius (radii?) from the
__main-containerelement
Outside of that, can you create a CSS vars for these new values so that folks can customize the variable and change the styles in this viewport size? What that might look like here is (with xs as the "xs" breakpoint, which is "0")
// add these at the top of the component styles with the other __main-container vars
--#{$page}__main-container--xs--MarginInlineStart: 0;
--#{$page}__main-container--xs-MarginInlineEnd: 0;
--#{$page}__main-container--xs--MaxHeight: 100%;
--#{$page}__main-container--xs--BorderRadius: 0;
// Update the styles in the breakpoint block you added
@media ... {
--#{$page}__main-container--MarginInlineStart: var(--#{$page}__main-container--xs--MarginInlineStart);
--#{$page}__main-container--MarginInlineEnd: var(--#{$page}__main-container--xs-MarginInlineEnd);
--#{$page}__main-container--MaxHeight: var(--#{$page}__main-container--xs--MaxHeight);
--#{$page}__main-container--BorderRadius: var(--#{$page}__main-container--xs--BorderRadius);
}There was a problem hiding this comment.
Love this change! Thank you @logonoff. Here is a small visual regression report of page, table, and navigation component examples/demos. Note, not everything in the report is from this change, so ignore the stuff that doesn't look related to this change.
Visual regression report (PDF)
@lboehling @andrew-ronaldson question around the small white border we add to the main content - should we also remove that border on the xs/sm breakpoints? I'm assuming so, but want to verify.
Here are a couple of screenshots that show the border. Note that the page section background and the content (when in a no-padding page section) do not touch the edge of the viewport.
There was a problem hiding this comment.
Confirmed with design that we'll leave the border 🚀
|
🎉 This PR is included in version 6.2.0-prerelease.19 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #7300
Before:
After:
