-
Notifications
You must be signed in to change notification settings - Fork 147
fix(components): width limit the description section #3237
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
Conversation
|
PF4 preview: https://patternfly-org-pr-3237-v4.surge.sh/v4 |
evwilkin
left a comment
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 - thanks!
mcoker
left a comment
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 great. I reviewed with @mceledonia and @lboehling and agreed the title and description should just use our default content/text styling. Then we can remove the extra section with the inline padding style.
WDYT about the community page and the footers?
mcoker
left a comment
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.
Looks awesome, just one thing it looks like.
| export const Footer = () => ( | ||
| <React.Fragment> | ||
| <PageSection key="footer-1" className="ws-org-pfsite-l-footer"> | ||
| <PageSection isWidthLimited key="footer-1" className="ws-org-pfsite-l-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.
Looks like this is creating some padding action for this block since there are custom padding styles defined here
patternfly-org/packages/documentation-framework/components/footer/footer.css
Lines 1 to 9 in 5f9afc2
| .ws-org-pfsite-l-footer.pf-c-page__main-section { | |
| font-family: "RedHatText"; | |
| font-weight: 300; | |
| background-color: #1a1a1a !important; | |
| padding: 48px; | |
| padding-top: 32px; | |
| /* Hide long overflowing tocs */ | |
| z-index: 1; | |
| } |
Easiest fix if we want to leave it the way it is would probably just be to set the right vars there, since the body element inherits the main-section's padding vars.
So like:
--pf-c-page__main-section--PaddingTop: var(--pf-global--spacer--xl);
--pf-c-page__main-section--PaddingRight: var(--pf-global--spacer--2xl);
--pf-c-page__main-section--PaddingBottom: var(--pf-global--spacer--2xl);
--pf-c-page__main-section--PaddingLeft: var(--pf-global--spacer--2xl);
mcoker
left a comment
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.
Just poking around, I think this looks a little odd on the home page since the home page content is centered. Maybe that's OK?
Here is what it looks like if we use the isCenterAligned prop
This is what it looks like with that set on component pages - wdyt @mceledonia @lboehling?
FWIW it's currently centered on component pages, it just grows to fill the viewport width.
42f2466 to
378e5c3
Compare
378e5c3 to
83ea73f
Compare
mcoker
left a comment
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!
evwilkin
left a comment
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 - thanks!







Fixes #3184
Adds IsWidthLimited to the description section, but since that adds a
pf-c-page__main-bodywith its own padding, I had to remove the utility class on the page section and instead directly modify the variable to remove top padding onpf-c-page__main-body.