Skip to content
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

chore(Toolbar): add different gap props to examples #11440

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mfrances17
Copy link
Contributor

What: Closes #10432

@mfrances17 mfrances17 requested review from thatblindgeye and a team January 17, 2025 23:26
@mfrances17 mfrances17 self-assigned this Jan 17, 2025
@mfrances17 mfrances17 requested review from wise-king-sullyman and removed request for a team January 17, 2025 23:26
@patternfly-build
Copy link
Contributor

patternfly-build commented Jan 17, 2025

@thatblindgeye thatblindgeye requested a review from a team January 20, 2025 18:12
@thatblindgeye thatblindgeye requested a review from mcoker January 20, 2025 18:43
Copy link
Contributor

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mfrances17 mfrances17 force-pushed the toolbar-spacer-examples branch from b075c2a to f6b2190 Compare January 23, 2025 19:16
Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

@mfrances17 mfrances17 force-pushed the toolbar-spacer-examples branch from c595773 to fefa4b4 Compare January 24, 2025 22:25
@mfrances17
Copy link
Contributor Author

@mcoker agreed on the title change, i changed it. i also removed the action groups because you're right, why overcomplicate the example.
@thatblindgeye @mcoker as for the layout, options are limited and i originally wanted to keep the examples consolidated into one single example file, but compromised and wound up separating them into two examples (toolbar group and toolbar item) to take advantage of the markdown headings. i also added them into their own heading at the bottom, much like was done with the toggle groups and filters examples. layout-wise, it works better imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toolbar - add examples showing new gap props
5 participants