-
Notifications
You must be signed in to change notification settings - Fork 375
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 4 commits
8ee57f2
834d7e4
923dff4
b8f9f6e
b5a2db6
07fa8fd
44fe096
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 | ||
| }), | ||
| {} | ||
| ), | ||
|
||
| 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 */ | ||
|
||
| 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 | ||
| }), | ||
| {} | ||
| ) | ||
|
||
| )} | ||
| {...(variant === 'label' && { 'aria-hidden': true })} | ||
| id={id} | ||
| role={role} | ||
| data-testid="toolbaritem" | ||
|
||
| {...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
Contributor
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.