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

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

Merged
merged 6 commits into from
May 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ A 0 0 0 0 1, 312.5, 85
index="0"
role="presentation"
shape-rendering="auto"
style="fill: var(--pf-v6-chart-bullet--qualitative-range--ColorScale--200, #92c5f9); stroke: var(--pf-v6-chart-bar--data--stroke, none); padding: 8px; stroke-width: 0;"
style="fill: var(--pf-v6-chart-bullet--qualitative-range--ColorScale--200, #e0e0e0); stroke: var(--pf-v6-chart-bar--data--stroke, none); padding: 8px; stroke-width: 0;"
/>
</g>
<g
Expand All @@ -399,7 +399,7 @@ A 0 0 0 0 1, 225, 85
index="0"
role="presentation"
shape-rendering="auto"
style="fill: var(--pf-v6-chart-bullet--qualitative-range--ColorScale--100, #92c5f9); stroke: var(--pf-v6-chart-bar--data--stroke, none); padding: 8px; stroke-width: 0;"
style="fill: var(--pf-v6-chart-bullet--qualitative-range--ColorScale--100, #f2f2f2); stroke: var(--pf-v6-chart-bar--data--stroke, none); padding: 8px; stroke-width: 0;"
/>
</g>
<g
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ A 0 0 0 0 1, 399.99999999976666, 104.8
index="0"
role="presentation"
shape-rendering="auto"
style="fill: var(--pf-v6-chart-bullet--qualitative-range--ColorScale--300, #92c5f9); stroke: var(--pf-v6-chart-bar--data--stroke, none); padding: 8px; stroke-width: 0;"
style="fill: var(--pf-v6-chart-bullet--qualitative-range--ColorScale--300, #c7c7c7); stroke: var(--pf-v6-chart-bar--data--stroke, none); padding: 8px; stroke-width: 0;"
/>
</g>
<g
Expand All @@ -102,7 +102,7 @@ A 0 0 0 0 1, 399.9999999995882, 104.8
index="0"
role="presentation"
shape-rendering="auto"
style="fill: var(--pf-v6-chart-bullet--qualitative-range--ColorScale--200, #92c5f9); stroke: var(--pf-v6-chart-bar--data--stroke, none); padding: 8px; stroke-width: 0;"
style="fill: var(--pf-v6-chart-bullet--qualitative-range--ColorScale--200, #e0e0e0); stroke: var(--pf-v6-chart-bar--data--stroke, none); padding: 8px; stroke-width: 0;"
/>
</g>
<g
Expand All @@ -121,7 +121,7 @@ A 0 0 0 0 1, 399.9999999993, 104.8
index="0"
role="presentation"
shape-rendering="auto"
style="fill: var(--pf-v6-chart-bullet--qualitative-range--ColorScale--100, #92c5f9); stroke: var(--pf-v6-chart-bar--data--stroke, none); padding: 8px; stroke-width: 0;"
style="fill: var(--pf-v6-chart-bullet--qualitative-range--ColorScale--100, #f2f2f2); stroke: var(--pf-v6-chart-bar--data--stroke, none); padding: 8px; stroke-width: 0;"
/>
</g>
</svg>
Expand Down
2 changes: 1 addition & 1 deletion packages/react-core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
"tslib": "^2.5.0"
},
"devDependencies": {
"@patternfly/patternfly": "6.0.0-alpha.135",
"@patternfly/patternfly": "6.0.0-alpha.139",
"@rollup/plugin-commonjs": "^25.0.0",
"@rollup/plugin-node-resolve": "^15.0.2",
"@rollup/plugin-replace": "^5.0.2",
Expand Down
136 changes: 118 additions & 18 deletions packages/react-core/src/components/Toolbar/ToolbarGroup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,119 @@ export interface ToolbarGroupProps extends Omit<React.HTMLProps<HTMLDivElement>,
alignItems?: 'start' | 'center' | 'baseline' | 'default';
/** Vertical alignment */
alignSelf?: 'start' | 'center' | 'baseline' | 'default';
/** Spacers at various breakpoints. */
spacer?: {
default?: 'spacerNone' | 'spacerSm' | 'spacerMd' | 'spacerLg';
md?: 'spacerNone' | 'spacerSm' | 'spacerMd' | 'spacerLg';
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. */
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could this be tackled as part of #7594?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

gap?: {
default?: 'gapNone' | 'gapXs' | 'gapSm' | 'gapMd' | 'gapLg' | 'gapXl' | 'gap_2xl' | 'gap_3xl' | 'gap_4xl';
md?: 'gapNone' | 'gapXs' | 'gapSm' | 'gapMd' | 'gapLg' | 'gapXl' | 'gap_2xl' | 'gap_3xl' | 'gap_4xl';
lg?: 'gapNone' | 'gapXs' | 'gapSm' | 'gapMd' | 'gapLg' | 'gapXl' | 'gap_2xl' | 'gap_3xl' | 'gap_4xl';
xl?: 'gapNone' | 'gapXs' | 'gapSm' | 'gapMd' | 'gapLg' | 'gapXl' | 'gap_2xl' | 'gap_3xl' | 'gap_4xl';
'2xl'?: 'gapNone' | 'gapXs' | 'gapSm' | 'gapMd' | 'gapLg' | 'gapXl' | 'gap_2xl' | 'gap_3xl' | 'gap_4xl';
};
/** Space items at various breakpoints. */
spaceItems?: {
default?: 'spaceItemsNone' | 'spaceItemsSm' | 'spaceItemsMd' | 'spaceItemsLg';
md?: 'spaceItemsNone' | 'spaceItemsSm' | 'spaceItemsMd' | 'spaceItemsLg';
lg?: 'spaceItemsNone' | 'spaceItemsSm' | 'spaceItemsMd' | 'spaceItemsLg';
xl?: 'spaceItemsNone' | 'spaceItemsSm' | 'spaceItemsMd' | 'spaceItemsLg';
'2xl'?: 'spaceItemsNone' | 'spaceItemsSm' | 'spaceItemsMd' | 'spaceItemsLg';
/** Sets only the column gap at various breakpoints. */
columnGap?: {
default?:
| 'columnGapNone'
| 'columnGapXs'
| 'columnGapSm'
| 'columnGapMd'
| 'columnGapLg'
| 'columnGapXl'
| 'columnGap_2xl'
| 'columnGap_3xl'
| 'columnGap_4xl';
Comment on lines +54 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

md?:
| 'columnGapNone'
| 'columnGapXs'
| 'columnGapSm'
| 'columnGapMd'
| 'columnGapLg'
| 'columnGapXl'
| 'columnGap_2xl'
| 'columnGap_3xl'
| 'columnGap_4xl';
lg?:
| 'columnGapNone'
| 'columnGapXs'
| 'columnGapSm'
| 'columnGapMd'
| 'columnGapLg'
| 'columnGapXl'
| 'columnGap_2xl'
| 'columnGap_3xl'
| 'columnGap_4xl';
xl?:
| 'columnGapNone'
| 'columnGapXs'
| 'columnGapSm'
| 'columnGapMd'
| 'columnGapLg'
| 'columnGapXl'
| 'columnGap_2xl'
| 'columnGap_3xl'
| 'columnGap_4xl';
'2xl'?:
| 'columnGapNone'
| 'columnGapXs'
| 'columnGapSm'
| 'columnGapMd'
| 'columnGapLg'
| 'columnGapXl'
| 'columnGap_2xl'
| 'columnGap_3xl'
| 'columnGap_4xl';
};
/** Sets only the row gap at various breakpoints. */
rowGap?: {
default?:
| 'rowGapNone'
| 'rowGapXs'
| 'rowGapSm'
| 'rowGapMd'
| 'rowGapLg'
| 'rowGapXl'
| 'rowGap_2xl'
| 'rowGao_3xl'
| 'rowGap_4xl';
md?:
| 'rowGapNone'
| 'rowGapXs'
| 'rowGapSm'
| 'rowGapMd'
| 'rowGapLg'
| 'rowGapXl'
| 'rowGap_2xl'
| 'rowGao_3xl'
| 'rowGap_4xl';
lg?:
| 'rowGapNone'
| 'rowGapXs'
| 'rowGapSm'
| 'rowGapMd'
| 'rowGapLg'
| 'rowGapXl'
| 'rowGap_2xl'
| 'rowGao_3xl'
| 'rowGap_4xl';
xl?:
| 'rowGapNone'
| 'rowGapXs'
| 'rowGapSm'
| 'rowGapMd'
| 'rowGapLg'
| 'rowGapXl'
| 'rowGap_2xl'
| 'rowGao_3xl'
| 'rowGap_4xl';
'2xl'?:
| 'rowGapNone'
| 'rowGapXs'
| 'rowGapSm'
| 'rowGapMd'
| 'rowGapLg'
| 'rowGapXl'
| 'rowGap_2xl'
| 'rowGao_3xl'
| 'rowGap_4xl';
};
/** Content to be rendered inside the data toolbar group */
children?: React.ReactNode;
Expand All @@ -65,8 +163,9 @@ class ToolbarGroupWithRef extends React.Component<ToolbarGroupProps> {
align,
alignItems,
alignSelf,
spacer,
spaceItems,
gap,
columnGap,
rowGap,
className,
variant,
children,
Expand All @@ -84,8 +183,9 @@ class ToolbarGroupWithRef extends React.Component<ToolbarGroupProps> {
variant && styles.modifiers[toCamel(variant) as 'filterGroup' | 'iconButtonGroup'],
formatBreakpointMods(visibility, styles, '', getBreakpoint(width)),
formatBreakpointMods(align, styles, '', getBreakpoint(width)),
formatBreakpointMods(spacer, styles, '', getBreakpoint(width)),
formatBreakpointMods(spaceItems, styles, '', getBreakpoint(width)),
formatBreakpointMods(gap, styles, '', getBreakpoint(width)),
formatBreakpointMods(columnGap, styles, '', getBreakpoint(width)),
formatBreakpointMods(rowGap, styles, '', getBreakpoint(width)),
alignItems === 'start' && styles.modifiers.alignItemsStart,
alignItems === 'center' && styles.modifiers.alignItemsCenter,
alignItems === 'baseline' && styles.modifiers.alignItemsBaseline,
Expand Down
128 changes: 119 additions & 9 deletions packages/react-core/src/components/Toolbar/ToolbarItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,119 @@ export interface ToolbarItemProps extends React.HTMLProps<HTMLDivElement> {
alignItems?: 'start' | 'center' | 'baseline' | 'default';
/** Vertical alignment */
alignSelf?: 'start' | 'center' | 'baseline' | 'default';
/** Spacers at various breakpoints. */
spacer?: {
default?: 'spacerNone' | 'spacerSm' | 'spacerMd' | 'spacerLg';
md?: 'spacerNone' | 'spacerSm' | 'spacerMd' | 'spacerLg';
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. */
gap?: {
default?: 'gapNone' | 'gapXs' | 'gapSm' | 'gapMd' | 'gapLg' | 'gapXl' | 'gap_2xl' | 'gap_3xl' | 'gap_4xl';
md?: 'gapNone' | 'gapXs' | 'gapSm' | 'gapMd' | 'gapLg' | 'gapXl' | 'gap_2xl' | 'gap_3xl' | 'gap_4xl';
lg?: 'gapNone' | 'gapXs' | 'gapSm' | 'gapMd' | 'gapLg' | 'gapXl' | 'gap_2xl' | 'gap_3xl' | 'gap_4xl';
xl?: 'gapNone' | 'gapXs' | 'gapSm' | 'gapMd' | 'gapLg' | 'gapXl' | 'gap_2xl' | 'gap_3xl' | 'gap_4xl';
'2xl'?: 'gapNone' | 'gapXs' | 'gapSm' | 'gapMd' | 'gapLg' | 'gapXl' | 'gap_2xl' | 'gap_3xl' | 'gap_4xl';
};
/** Sets only the column gap at various breakpoints. */
columnGap?: {
default?:
| 'columnGapNone'
| 'columnGapXs'
| 'columnGapSm'
| 'columnGapMd'
| 'columnGapLg'
| 'columnGapXl'
| 'columnGap_2xl'
| 'columnGap_3xl'
| 'columnGap_4xl';
md?:
| 'columnGapNone'
| 'columnGapXs'
| 'columnGapSm'
| 'columnGapMd'
| 'columnGapLg'
| 'columnGapXl'
| 'columnGap_2xl'
| 'columnGap_3xl'
| 'columnGap_4xl';
lg?:
| 'columnGapNone'
| 'columnGapXs'
| 'columnGapSm'
| 'columnGapMd'
| 'columnGapLg'
| 'columnGapXl'
| 'columnGap_2xl'
| 'columnGap_3xl'
| 'columnGap_4xl';
xl?:
| 'columnGapNone'
| 'columnGapXs'
| 'columnGapSm'
| 'columnGapMd'
| 'columnGapLg'
| 'columnGapXl'
| 'columnGap_2xl'
| 'columnGap_3xl'
| 'columnGap_4xl';
'2xl'?:
| 'columnGapNone'
| 'columnGapXs'
| 'columnGapSm'
| 'columnGapMd'
| 'columnGapLg'
| 'columnGapXl'
| 'columnGap_2xl'
| 'columnGap_3xl'
| 'columnGap_4xl';
};
/** Sets only the row gap at various breakpoints. */
rowGap?: {
default?:
| 'rowGapNone'
| 'rowGapXs'
| 'rowGapSm'
| 'rowGapMd'
| 'rowGapLg'
| 'rowGapXl'
| 'rowGap_2xl'
| 'rowGao_3xl'
| 'rowGap_4xl';
md?:
| 'rowGapNone'
| 'rowGapXs'
| 'rowGapSm'
| 'rowGapMd'
| 'rowGapLg'
| 'rowGapXl'
| 'rowGap_2xl'
| 'rowGao_3xl'
| 'rowGap_4xl';
lg?:
| 'rowGapNone'
| 'rowGapXs'
| 'rowGapSm'
| 'rowGapMd'
| 'rowGapLg'
| 'rowGapXl'
| 'rowGap_2xl'
| 'rowGao_3xl'
| 'rowGap_4xl';
xl?:
| 'rowGapNone'
| 'rowGapXs'
| 'rowGapSm'
| 'rowGapMd'
| 'rowGapLg'
| 'rowGapXl'
| 'rowGap_2xl'
| 'rowGao_3xl'
| 'rowGap_4xl';
'2xl'?:
| 'rowGapNone'
| 'rowGapXs'
| 'rowGapSm'
| 'rowGapMd'
| 'rowGapLg'
| 'rowGapXl'
| 'rowGap_2xl'
| 'rowGao_3xl'
| 'rowGap_4xl';
};
/** id for this data toolbar item */
id?: string;
Expand All @@ -60,7 +166,9 @@ export const ToolbarItem: React.FunctionComponent<ToolbarItemProps> = ({
className,
variant,
visibility,
spacer,
gap,
columnGap,
rowGap,
align,
alignSelf,
alignItems,
Expand All @@ -86,7 +194,9 @@ export const ToolbarItem: React.FunctionComponent<ToolbarItemProps> = ({
isOverflowContainer && styles.modifiers.overflowContainer,
formatBreakpointMods(visibility, styles, '', getBreakpoint(width)),
formatBreakpointMods(align, styles, '', getBreakpoint(width)),
formatBreakpointMods(spacer, styles, '', getBreakpoint(width)),
formatBreakpointMods(gap, styles, '', getBreakpoint(width)),
formatBreakpointMods(columnGap, styles, '', getBreakpoint(width)),
formatBreakpointMods(rowGap, styles, '', getBreakpoint(width)),
alignItems === 'start' && styles.modifiers.alignItemsStart,
alignItems === 'center' && styles.modifiers.alignItemsCenter,
alignItems === 'baseline' && styles.modifiers.alignItemsBaseline,
Expand Down
Loading
Loading