-
Notifications
You must be signed in to change notification settings - Fork 373
fix(Toolbar): add width and flex-grow props to ToolbarItem; add flex-grow to ToolbarGroup; update demo and tests #12074
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
base: main
Are you sure you want to change the base?
Changes from all commits
8ee57f2
834d7e4
923dff4
b8f9f6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,7 @@ | ||
| httpRetry: 5 | ||
|
|
||
| httpTimeout: 600000 | ||
|
|
||
| nodeLinker: node-modules | ||
|
|
||
| npmRegistryServer: "https://registry.npmjs.org" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
|
|
||
| function deleteFolderRecursive(pathToDelete) { | ||
| if (fs.existsSync(pathToDelete)) { | ||
| fs.readdirSync(pathToDelete).forEach((file) => { | ||
| const curPath = path.join(pathToDelete, file); | ||
| if (fs.lstatSync(curPath).isDirectory()) { | ||
| deleteFolderRecursive(curPath); | ||
| } else { | ||
| try { | ||
| fs.unlinkSync(curPath); | ||
| } catch (err) { | ||
| // console.error(`Error deleting file ${curPath}:`, err); | ||
| } | ||
| } | ||
| }); | ||
| try { | ||
| fs.rmdirSync(pathToDelete); | ||
| } catch (err) { | ||
| // console.error(`Error deleting directory ${pathToDelete}:`, err); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ['node_modules', '.yarn'].forEach((dir) => { | ||
| // console.log(`Removing ${dir}...`); | ||
| deleteFolderRecursive(dir); | ||
| }); | ||
|
|
||
| ['.pnp.cjs', '.pnp.loader.mjs'].forEach((file) => { | ||
| if (fs.existsSync(file)) { | ||
| try { | ||
| fs.unlinkSync(file); | ||
| // console.log(`Removed ${file}`); | ||
| } catch (err) { | ||
| // console.error(`Error removing ${file}:`, err); | ||
| } | ||
| } | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -166,6 +166,15 @@ export interface ToolbarGroupProps extends Omit<React.HTMLProps<HTMLDivElement>, | |
| xl?: 'wrap' | 'nowrap'; | ||
| '2xl'?: 'wrap' | 'nowrap'; | ||
| }; | ||
| /** Flex grow modifier at various breakpoints */ | ||
| flexGrow?: { | ||
| default?: 'flexGrow'; | ||
| sm?: 'flexGrow'; | ||
| md?: 'flexGrow'; | ||
| lg?: 'flexGrow'; | ||
| xl?: 'flexGrow'; | ||
| '2xl'?: 'flexGrow'; | ||
| }; | ||
| /** Content to be rendered inside the data toolbar group */ | ||
| children?: React.ReactNode; | ||
| /** Flag that modifies the toolbar group to hide overflow and respond to available space. Used for horizontal navigation. */ | ||
|
|
@@ -185,6 +194,7 @@ class ToolbarGroupWithRef extends Component<ToolbarGroupProps> { | |
| columnGap, | ||
| rowGap, | ||
| rowWrap, | ||
| flexGrow, | ||
| className, | ||
| variant, | ||
| children, | ||
|
|
@@ -221,6 +231,14 @@ class ToolbarGroupWithRef extends Component<ToolbarGroupProps> { | |
| alignSelf === 'center' && styles.modifiers.alignSelfCenter, | ||
| alignSelf === 'baseline' && styles.modifiers.alignSelfBaseline, | ||
| isOverflowContainer && styles.modifiers.overflowContainer, | ||
| flexGrow && | ||
| Object.entries(flexGrow).reduce( | ||
| (acc, [bp]) => ({ | ||
| ...acc, | ||
| [`pf-m-flex-grow${bp !== 'default' ? `-on-${bp}` : ''}`]: true | ||
| }), | ||
| {} | ||
| ), | ||
|
Comment on lines
+234
to
+241
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than this, we can just use our formateBreakpointMods util function |
||
| className | ||
| )} | ||
| {...props} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,24 @@ export interface ToolbarItemProps extends React.HTMLProps<HTMLDivElement> { | |
| className?: string; | ||
| /** A type modifier which modifies spacing specifically depending on the type of item */ | ||
| variant?: ToolbarItemVariant | 'pagination' | 'label' | 'label-group' | 'separator' | 'expand-all'; | ||
| /** Width modifier at various breakpoints */ | ||
| widths?: { | ||
| default?: 'sm' | 'md' | 'lg' | 'xl' | '2xl' | '3xl' | '4xl'; | ||
| sm?: 'sm' | 'md' | 'lg' | 'xl' | '2xl' | '3xl' | '4xl'; | ||
| md?: 'sm' | 'md' | 'lg' | 'xl' | '2xl' | '3xl' | '4xl'; | ||
| lg?: 'sm' | 'md' | 'lg' | 'xl' | '2xl' | '3xl' | '4xl'; | ||
| xl?: 'sm' | 'md' | 'lg' | 'xl' | '2xl' | '3xl' | '4xl'; | ||
| '2xl'?: 'sm' | 'md' | 'lg' | 'xl' | '2xl' | '3xl' | '4xl'; | ||
| }; | ||
| /** Flex grow modifier at various breakpoints */ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar nit here for prop description as above |
||
| flexGrow?: { | ||
| default?: 'flexGrow'; | ||
| sm?: 'flexGrow'; | ||
| md?: 'flexGrow'; | ||
| lg?: 'flexGrow'; | ||
| xl?: 'flexGrow'; | ||
| '2xl'?: 'flexGrow'; | ||
| }; | ||
| /** Visibility at various breakpoints. */ | ||
| visibility?: { | ||
| default?: 'hidden' | 'visible'; | ||
|
|
@@ -185,6 +203,8 @@ export const ToolbarItem: React.FunctionComponent<ToolbarItemProps> = ({ | |
| children, | ||
| isAllExpanded, | ||
| isOverflowContainer, | ||
| widths, | ||
| flexGrow, | ||
| role, | ||
| ...props | ||
| }: ToolbarItemProps) => { | ||
|
|
@@ -210,23 +230,40 @@ export const ToolbarItem: React.FunctionComponent<ToolbarItemProps> = ({ | |
| variant === ToolbarItemVariant['label-group'] && styles.modifiers.labelGroup, | ||
| isAllExpanded && styles.modifiers.expanded, | ||
| isOverflowContainer && styles.modifiers.overflowContainer, | ||
| formatBreakpointMods(visibility, styles, '', getBreakpoint(width)), | ||
| formatBreakpointMods(align, styles, '', getBreakpoint(width)), | ||
| formatBreakpointMods(gap, styles, '', getBreakpoint(width)), | ||
| formatBreakpointMods(columnGap, styles, '', getBreakpoint(width)), | ||
| formatBreakpointMods(rowGap, styles, '', getBreakpoint(width)), | ||
| formatBreakpointMods(rowWrap, styles, '', getBreakpoint(width)), | ||
| alignItems === 'start' && styles.modifiers.alignItemsStart, | ||
| alignItems === 'center' && styles.modifiers.alignItemsCenter, | ||
| alignItems === 'baseline' && styles.modifiers.alignItemsBaseline, | ||
| alignSelf === 'start' && styles.modifiers.alignSelfStart, | ||
| alignSelf === 'center' && styles.modifiers.alignSelfCenter, | ||
| alignSelf === 'baseline' && styles.modifiers.alignSelfBaseline, | ||
| className | ||
| formatBreakpointMods(visibility, styles, '', getBreakpoint(width)), | ||
| formatBreakpointMods(align, styles, '', getBreakpoint(width)), | ||
| formatBreakpointMods(gap, styles, '', getBreakpoint(width)), | ||
| formatBreakpointMods(columnGap, styles, '', getBreakpoint(width)), | ||
| formatBreakpointMods(rowGap, styles, '', getBreakpoint(width)), | ||
| formatBreakpointMods(rowWrap, styles, '', getBreakpoint(width)), | ||
| className, | ||
| widths && | ||
| Object.entries(widths).reduce( | ||
| (acc, [bp, size]) => ({ | ||
| ...acc, | ||
| [`pf-m-w-${size}${bp !== 'default' ? `-on-${bp}` : ''}`]: true | ||
| }), | ||
| {} | ||
| ), | ||
| flexGrow && | ||
| Object.entries(flexGrow).reduce( | ||
| (acc, [bp]) => ({ | ||
| ...acc, | ||
| [`pf-m-flex-grow${bp !== 'default' ? `-on-${bp}` : ''}`]: true | ||
| }), | ||
| {} | ||
| ) | ||
|
Comment on lines
+246
to
+261
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to above, we should be able to just use our formatBreakpointMods util function instead There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just an addendum to this, we should use the formatBreakpointMods function for the flexGrow here, but for handling applying the widths prop we should basically just add back in what was removed in this React PR https://github.com/patternfly/patternfly-react/pull/10022/files#diff-7beeea05e52252eb360f045d661fab89fe634b00becfd8a3dbaa48338c6032c0 (we want to apply a |
||
| )} | ||
| {...(variant === 'label' && { 'aria-hidden': true })} | ||
| id={id} | ||
| role={role} | ||
| data-testid="toolbaritem" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about hardcoding a testid in, since there could be many items on a page, and a testid should be able to be passed with the spread props line below this. |
||
| {...props} | ||
| > | ||
| {children} | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,22 @@ describe('ToolbarGroup', () => { | |||||||||||||||||||||||||||
| expect(screen.getByTestId('toolbargroup')).toHaveClass('pf-m-overflow-container'); | ||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| describe('ToolbarGroup flexGrow', () => { | ||||||||||||||||||||||||||||
| const bps = ['default', 'sm', 'md', 'lg', 'xl', '2xl']; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| describe.each(bps)('flexGrow at various breakpoints', (bp) => { | ||||||||||||||||||||||||||||
| it(`should render with pf-m-flex-grow when flexGrow is set at ${bp}`, () => { | ||||||||||||||||||||||||||||
|
Comment on lines
+14
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We typically use the describe block a bit more rarely in our codebase, in this case I'm not sure we should use it. Instead what we could do is put the breakpoints variable as a global var or within the first
We do something similar in some of our unit test files, such as our Button tests: patternfly-react/packages/react-core/src/components/Button/__tests__/Button.test.tsx Lines 10 to 22 in 9421a92
|
||||||||||||||||||||||||||||
| render( | ||||||||||||||||||||||||||||
| <ToolbarGroup data-testid="toolbargroup" flexGrow={{ [bp]: 'flexGrow' }}> | ||||||||||||||||||||||||||||
| Test | ||||||||||||||||||||||||||||
| </ToolbarGroup> | ||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
| const bpFlexGrowClass = bp === 'default' ? 'pf-m-flex-grow' : `pf-m-flex-grow-on-${bp}`; | ||||||||||||||||||||||||||||
| expect(screen.getByTestId('toolbargroup')).toHaveClass(bpFlexGrowClass); | ||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| describe('ToobarGroup rowWrap', () => { | ||||||||||||||||||||||||||||
| const bps = ['default', 'sm', 'md', 'lg', 'xl', '2xl']; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
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.
Small nit to the description, just to make it clear that it's currently a hardcoded flex grow modifier rather than something more customizable.