-
Notifications
You must be signed in to change notification settings - Fork 20
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
fix(build): update to PF6 betas #228
Conversation
Preview: https://react-topology-pr-topology-228.surge.sh Demo-app: https://v6-betas.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.
@jenny-s51 Looks like snap shots need updating.
c850142
to
a0f7809
Compare
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.
Updated to fix UI issues in the demo app, added changes to PR description.
@jeff-phillips-18 curious to get your thoughts, Topology Package and Styles -> Labels demos are cutting off the bottom part of the graph view with these updates. Is there a way to load in the graph at a smaller size for those demos?
@jenny-s51 This is no different from the past. We could decrease the space between the rows if we wanted but it is really just dependent on the browser height. The user can pan the view to see those items not in view. |
Thank you @jeff-phillips-18 for your clarity here. Marking this PR as ready for review with the existing updates cc @dlabaj @nicolethoen |
packages/demo-app-ts/src/Demo.css
Outdated
@@ -77,4 +77,10 @@ | |||
|
|||
.pf-v6-c-toolbar__content.pf-m-hidden { | |||
display: none; | |||
} | |||
|
|||
.pf-v6-c-page__main-container, |
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 think we introduced a prop that applies these styles to page sections. Let me ask the scrum channel
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.
@jenny-s51 here's the response I got from @mcoker
The general setup is, if you have a page section(s) you want to fill the available space, you add isContentFilled to and that tells the container that wraps the page content to fill the viewport (the white background will extend the height of the viewport). Then you need to add isFilled to any page sections you want to fill that space.
Here's an example w/ those deets - https://staging-v6.patternfly.org/components/page#filled-page-sections
and here's a codesandbox - https://codesandbox.io/s/sweet-frog-pp7zcr?file=/index.tsx:1761-1776
(FWIW the old behavior in v5 was that the main content area always filled the viewport height, and the last page section (whichever matches :last-child) would be filled by default. If you wanted to, for example, have the first page section fill the viewport instead, you'd add isFilled to the first page section, then add isFilled={false} to the last page section
If you see isFilled={false} that's probably why. That's no longer needed in v6)
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.
Nice catch, thank you @nicolethoen. I asked @mcoker a follow-up question in the Slack thread after applying these suggested changes , curious to get his thoughts on this. We want a StackItem
to fill the height of the PageSection
and currently without the height set on .pf-v6-c-page__main-body
, the fill does not get applied even though StackItem
has the pf-m-fill
class as well.
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.
Removed the custom styling on the page components - added hasBodyWrapper={false}
to the filled <PageSections>
per @mcoker 's suggestion which resolved the issue
@@ -23,6 +22,7 @@ | |||
.pf-ri__topology-demo .pf-v6-c-toolbar { | |||
--pf-v6-c-toolbar__expandable-content--lg--PaddingBottom: 0; | |||
--pf-v6-c-toolbar--PaddingBottom: 0; | |||
--pf-v6-c-toolbar--PaddingBlockStart: var(--pf-t--global--spacer--300); |
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 see some references to base tokens in this CSS. Ideally we would use semantic tokens without --300
or other numbers at the end.
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 @nicolethoen ! Updated to use semantic tokens with the exception of --pf-t--color--red--50
, which I couldn't find referenced with any semantic tokens in https://github.com/patternfly/patternfly/blob/v6/src/patternfly/base/tokens
@jeff-phillips-18 are you using a flex layout in the toolbar? |
@nicolethoen yes we do wrap the toolbar items in a |
It is using |
if you remove the Split, does it look better? |
Yes. Changing the Just kinda wondering why the difference between v5 and v6? |
Lots of spacing was streamlined and standardized to use the tokens. So more components should aligns and be spaced better out of the box 👍🏻 |
<Split> | ||
<SplitItem> | ||
<Flex flexWrap={{ default: 'nowrap' }} gap={{ default: 'gapSm' }} alignItems={{ default: 'alignItemsCenter' }}> | ||
<FlexItem> |
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.
What if you don't have a flex layouts in the toolbar at all?
you could use:
<ToolbarItem variant='label'>Layout:</ToolbarItem>
<ToolbarItem'><Dropdown.../></ToolbarItem>
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.
Working on refactoring these instances where we use Flex layouts to use the syntax above
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 reason I used a single ToolbarItem
with a Flex
for the label and input was so that the label and input would look more related (less space between them than between the input and the next label). Also, if the widow width is less, the new implementation cuts off the items rather than wrapping.
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 clarifying @jeff-phillips-18 , reverted back to your changes in jenny-s51#11
b39ada4
to
c9d584d
Compare
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
update PF icon, update styling to fix demo-app examples update snapshots fix brand height remove custom page styles, add props to fix page fill wip toolbar refactor for topology package update to semantic tokens refactor split and flex instances in favor of toolbar components remove unused split components revert toolbar refactor
🎉 This PR is included in version 6.0.0-alpha.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What
Update to beta versions.
Docs workspace looks good with PR updates (see below for generated link preview once build passes).
Demo app needs some CSS updates to viewport heights to fix the demos. This is a known issue - currently investigating and will push the updates here once resolved.Updates CSS to fix demo viewport heightsCan see the current state of the demo-app here: https://v6-betas.surge.sh/
Type of change