feat(Toolbar): updated spacer props to gap #10418

thatblindgeye · 2024-05-22T17:41:05Z

What: Closes #10387

Couple notes (in addition to the one below in another comment):

I imagine we'll want to add these same props onto ToolbarContent since Core supports it. Opted to not include it here since we don't currently, and the API may be even more verbose since we currently don't have a separate ContentSection component. So we may end up needing props like gap, sectionGap and so on.
May be worth a followup at some point (not necessarily for v6) to simplify the API in terms of these new/replacement props. `gapNone" etc end up verbose, and it'd be great if we could simplify it to just "none" and so on.

Additional issues:

thatblindgeye · 2024-05-22T17:43:52Z

packages/react-core/src/components/Toolbar/ToolbarGroup.tsx

+      | 'rowGap_4xl';
+  };
+  /** Sets the margin-inline-start at various breakpoints. */
+  marginInlineStart?: {


Just a note, right now this and the marginInlineEnd props won't work correctly due to the styles object casing. Instead of marginInlineStartNone for example, we need to pass marginInlineStartnone (lowercased "none").

I can update this PR to use the lowercased size, but I opted to keep the intended (?) prop values in case we can get a quick Core update in to maybe help resolve this

@thatblindgeye added a PR to fix patternfly/patternfly#6684 (cc @mattnolting)

patternfly-build · 2024-05-22T18:12:18Z

Preview: https://patternfly-react-pr-10418.surge.sh

A11y report: https://patternfly-react-pr-10418-a11y.surge.sh

mcoker

LGTM! One question about the use of _ in the prop values, though I'd like to get clarification from @mattnolting on a couple of things from https://github.com/patternfly/patternfly/pull/6057/files

Can you provide some detail on the .pf-m-margin* classes? They can provide the same functionality the current spacer system provides, but I don't see them included in the issue description or mentioned/used in the core docs, so want to make sure they're intentional before we add them to the react component and have users update to use them.
I imagine we'll want to add these same props onto ToolbarContent since Core supports it. Opted to not include it here since we don't currently, and the API may be even more verbose since we currently don't have a separate ContentSection component. So we may end up needing props like gap, sectionGap and so on.
- Is there any functionality for the toolbar and/or other components that use toolbar to add the spacer system to <ToolbarContent>? If not and it wasn't a requested feature/enhancement, I wonder if we can remove the CSS and just add that later if it's requested, since it may not be a straight forward addition to the react component.

mcoker · 2024-05-22T23:28:38Z

packages/react-core/src/components/Toolbar/ToolbarGroup.tsx

+      | 'columnGap_2xl'
+      | 'columnGap_3xl'
+      | 'columnGap_4xl';


Just curious about the _ in the values. I don't see them in the margin* props. And we have a very similar spacing system in the flex layout and the props/values look identical (at first glance) except for the _ and wondering if that should be consistent?

This should be due to the missing hyphen in the margin classnames. Whatever logic camel-case has (which react-styles is using to build these maps) essentially replaces hyphens with underscores if the order is letter + hyphen + number.

Hmm... I'm not sure I understand how these values are tied to the margin modifiers? And why only the ones that start with numbers are prefixed with an underscore.

mcoker · 2024-05-23T15:37:45Z

@thatblindgeye confirmed with @mattnolting that we don't want to include the .pf-m-margin* modifiers, and we can remove them from the core CSS before beta. If we can take them out of this PR, I think it's good-to-go!

mcoker

Just leaving an official review for the change to drop the marginInline* props from this comment #10418 (comment)

thatblindgeye · 2024-05-23T16:12:30Z

Followup regarding adding new examples: #10432

mcoker

LGTM! Great work @thatblindgeye and thanks for jumping on this so quickly.

tlabaj · 2024-05-23T18:56:20Z

packages/react-core/src/components/Toolbar/examples/Toolbar.md

@@ -28,7 +28,7 @@ Note: This example does not demonstrate responsive toolbar behavior. Responsive

 You may adjust the space between toolbar items to arrange them into groups. Read our spacers documentation to learn more about using spacers.

-Items are spaced “16px” apart by default. To adjust the size of the space between items, use the `spacer` property of each `<ToolbarItem>`. You can set the `spacer` value at multiple breakpoints, including "default", "md", "lg", "xl", and "2xl". Available `spacer` values include "spacerNone", "spacerSm", "spacerMd", or "spacerLg" into each breakpoint.
+Items are spaced “16px” apart by default via their parents' `gap` or `columnGap` property. You can set the property values at multiple breakpoints, including "default", "md", "lg", "xl", and "2xl".


should there be a blurb about rowGap somewhere?

I opened a followup for additional examples as well as updating this example: #10432

tlabaj · 2024-05-23T19:02:50Z

packages/react-core/src/components/Toolbar/ToolbarGroup.tsx

-    lg?: 'spacerNone' | 'spacerSm' | 'spacerMd' | 'spacerLg';
-    xl?: 'spacerNone' | 'spacerSm' | 'spacerMd' | 'spacerLg';
-    '2xl'?: 'spacerNone' | 'spacerSm' | 'spacerMd' | 'spacerLg';
+  /** Sets both the column and row gap at various breakpoints. */


Not a blocker, but I would update/ add unit test to verify that formatBreakpointMods formats all the values accurately.

Could this be tackled as part of #7594?

It can, as long as we at least test them manually.

tlabaj · 2024-05-23T19:04:19Z

packages/react-core/src/components/Toolbar/examples/ToolbarSpacers.tsx

@@ -5,34 +5,27 @@ import { Button } from '@patternfly/react-core';
 export const ToolbarSpacers: React.FunctionComponent = () => {
  const items = (
    <React.Fragment>
-      <ToolbarItem spacer={{ default: 'spacerNone' }}>
+      <ToolbarItem>


Nit. Should we rename this file to ToolbarGaps?

tlabaj

LGTM

evwilkin · 2024-05-23T20:31:20Z

@thatblindgeye @tlabaj see this just merged but wanted to note that all the 'rowGap_3xl' usages in this code have a typo of 'rowGao_3xl'

patternfly-build · 2024-05-23T20:37:44Z

Your changes have been released in:

@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]

Thanks for your contribution! 🎉

feat(Toolbar): updated spacer props to gap

64573c5

thatblindgeye requested review from mcoker, a team, tlabaj and kmcfaul and removed request for a team May 22, 2024 17:41

thatblindgeye commented May 22, 2024

View reviewed changes

Updated additional examples/demos

737e053

thatblindgeye added the require codemod label May 22, 2024

thatblindgeye mentioned this pull request May 22, 2024

Toolbar - updates spacers to gaps patternfly/pf-codemods#645

Closed

mcoker reviewed May 23, 2024

View reviewed changes

thatblindgeye linked an issue May 23, 2024 that may be closed by this pull request

Toolbar - spacer/gap updates #10387

Closed

5 tasks

mcoker requested changes May 23, 2024

View reviewed changes

mcoker mentioned this pull request May 23, 2024

fix(toolbar): typo in inline margin modifier classes patternfly/patternfly#6684

Closed

thatblindgeye added 2 commits May 23, 2024 12:07

Removed margin props

95c5160

Definitely did not forget to add underscore back

a6543ff

thatblindgeye mentioned this pull request May 23, 2024

Toolbar - add examples showing new gap props #10432

Open

mcoker approved these changes May 23, 2024

View reviewed changes

kmcfaul approved these changes May 23, 2024

View reviewed changes

tlabaj reviewed May 23, 2024

View reviewed changes

Updated core versions

fb84b90

tlabaj approved these changes May 23, 2024

View reviewed changes

Updated snapshots

0ba4415

tlabaj merged commit 29578a2 into patternfly:v6 May 23, 2024
13 checks passed

thatblindgeye mentioned this pull request May 23, 2024

Bug - Toolbar - typo in rowGap prop in v6 #10441

Closed

This was referenced May 23, 2024

Toolbar - spacer/gap updates #10387

Closed

Toolbar component review patternfly/patternfly#6701

Closed

thatblindgeye mentioned this pull request May 30, 2024

Toolbar v6 - update spacer/spaceItems to gap props patternfly/patternfly-org#4046

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Toolbar): updated spacer props to gap #10418

feat(Toolbar): updated spacer props to gap #10418

thatblindgeye commented May 22, 2024 •

edited

Loading

thatblindgeye May 22, 2024

mcoker May 22, 2024

patternfly-build commented May 22, 2024 •

edited

Loading

mcoker left a comment

mcoker May 22, 2024

thatblindgeye May 23, 2024

mcoker May 23, 2024

mcoker commented May 23, 2024

mcoker left a comment

thatblindgeye commented May 23, 2024

mcoker left a comment

tlabaj May 23, 2024

thatblindgeye May 23, 2024

tlabaj May 23, 2024

thatblindgeye May 23, 2024

tlabaj May 23, 2024

tlabaj May 23, 2024

tlabaj left a comment

evwilkin commented May 23, 2024

patternfly-build commented May 23, 2024

feat(Toolbar): updated spacer props to gap #10418

feat(Toolbar): updated spacer props to gap #10418

Conversation

thatblindgeye commented May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patternfly-build commented May 22, 2024 • edited Loading

mcoker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcoker commented May 23, 2024

mcoker left a comment

Choose a reason for hiding this comment

thatblindgeye commented May 23, 2024

mcoker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlabaj left a comment

Choose a reason for hiding this comment

evwilkin commented May 23, 2024

patternfly-build commented May 23, 2024

thatblindgeye commented May 22, 2024 •

edited

Loading

patternfly-build commented May 22, 2024 •

edited

Loading