chore(Toolbar): add different gap props to examples#11440
chore(Toolbar): add different gap props to examples#11440thatblindgeye merged 4 commits intopatternfly:mainfrom
Conversation
|
Preview: https://patternfly-react-pr-11440.surge.sh A11y report: https://patternfly-react-pr-11440-a11y.surge.sh |
packages/react-core/src/components/Toolbar/examples/ToolbarSpacers.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Toolbar/examples/ToolbarSpacers.tsx
Outdated
Show resolved
Hide resolved
wise-king-sullyman
left a comment
There was a problem hiding this comment.
I would maybe rather see us have these examples a bit DRYer, but I could also see a good argument for keeping it as is, so not a blocker.
b075c2a to
f6b2190
Compare
mcoker
left a comment
There was a problem hiding this comment.
LGTM! A couple of questions:
- I wonder if "Toolbar item spacers" would be better as "Toolbar spacers", especially since you have examples of using spacers on toolbar items now, too.
- I see
variant="action-group"on most of the<ToolbarGroup />s, I'm curious if there is a reason to use that over no variant? I'd consider no variant the default for example/demo purposes, but there are buttons in the example, and action-group is probably best for buttons. - I think the headings and hwhat-not could use some love. I wonder if this would be a good use case for subheadings? That would probably be a separate issue, but like the html docs for table. Maybe something like:
## Toolbar spacers
[...existing blurb]
### Toolbar group spacers
[gap]
[column gap]
[row gap]
### Toolbar item spacers
[gap]
[column gap]
[row gap]
c595773 to
fefa4b4
Compare
|
@mcoker agreed on the title change, i changed it. i also removed the action groups because you're right, why overcomplicate the example. |
thatblindgeye
left a comment
There was a problem hiding this comment.
I think this is looking great. Unless you'd want to do it as part of this PR, could you open a followup to add a prop for gap wrapping/nowrapping on these components + adding tests for the new class? I'd assume we'd want it setup similar to how the Flex layout component has the flexWrap prop, but will defer to @mcoker if there should be any difference within ToolbarGroup and ToolbarItem.
|
Filed #11489 as a follow-up to add a wrap prop to ToolbarGroup and ToolbarItem gap. |
mcoker
left a comment
There was a problem hiding this comment.
L🎃TM! The headings look great now as they are IMO.
I'd assume we'd want it setup similar to how the Flex layout component has the flexWrap prop, but will defer to @mcoker if there should be any difference within ToolbarGroup and ToolbarItem.
@thatblindgeye technically speaking, regarding flex props and what a wrap modifier would do for the toolbar compared to the flex layout, I think they're identical so 👍👍 from me
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
* chore(Toolbar): add different gap props to examples * use h4 for example headings * rm redundant wrap classes * reformat and restructure examples
What: Closes #10432