-
Notifications
You must be signed in to change notification settings - Fork 360
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(Toolbar): Add row wrap prop to ToolbarGroup and ToolbarItem #11559
base: main
Are you sure you want to change the base?
Conversation
Preview: https://patternfly-react-pr-11559.surge.sh A11y report: https://patternfly-react-pr-11559-a11y.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.
Just a couple of things:
- we don't support
.pf-m-wrap-reverse
.pf-m-wrap
and.pf-m-nowrap
applies to.pf-v6-c-toolbar__content-section
, too - https://github.com/patternfly/patternfly/blob/93efaf133c72a9e1944660bd404ad8b38f850d93/src/patternfly/components/Toolbar/toolbar.scss#L360-L369
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.
In addition to adding the prop to ToolbarContent (for the ContentSection class), could we add a simple example for that? Maybe we could update the section from "Examples with toolbar spacers" to "Examples with spacers and wrapping", then add a "Toolbar content wrapping" example to show the nowrap modifier in use (the example description could mention that by default the content section wraps).
Also can we add quick unit tests for these components for the new modifier as well
8f065d0
to
b1bd21d
Compare
thanks @mcoker & @thatblindgeye, i've added a wrap prop to |
What: Closes #11489