-
Notifications
You must be signed in to change notification settings - Fork 376
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 | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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'; | ||||||||||||||||||||||||||
|
Comment on lines
+23
to
+28
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. For this prop, we want the types to be just
Suggested change
|
||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| /** Flex grow modifier at various breakpoints */ | ||||||||||||||||||||||||||
thatblindgeye marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
| 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" | ||||||||||||||||||||||||||
thatblindgeye marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
| {...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']; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.