-
Notifications
You must be signed in to change notification settings - Fork 377
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
3577bd0
869eb6b
93919cd
f27e3d9
25e0844
051045a
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'; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
| 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'; | |
| default?: string; | |
| sm?: string; | |
| md?: string; | |
| lg?: string; | |
| xl?: string; | |
| '2xl'?: string; |
thatblindgeye marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
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.
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 comment
The 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 styles object rather than a class)
thatblindgeye marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| 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', () => { | ||||||||||||||||||||||||||||
|
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. Typo in describe block name: "ToobarGroup" should be "ToolbarGroup". 🔤 Suggested fix- describe('ToobarGroup rowWrap', () => {
+ describe('ToolbarGroup rowWrap', () => {📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
| const bps = ['default', 'sm', 'md', 'lg', 'xl', '2xl']; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.