fix(Toolbar): add width and flex-grow props to ToolbarItem; add flex-grow to ToolbarGroup; update demo and tests #12074
Conversation
|
Preview: https://pf-react-pr-12074.surge.sh A11y report: https://pf-react-pr-12074-a11y.surge.sh |
…exGrow; update demo and tests; fix lint
khadesamrudhi
changed the title
|
First-time contributor, could a maintainer please approve workflows and re-run the failed job? Notes:
Thanks! |
|
I’ve fixed the packages/tsconfig.base.json JSON issue and pushed the update. Could you please approve the workflows and re-run the CI? If Surge preview shows |
|
Hi, all automated checks have passed. |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Some comments below. Additionally, there are several files updates not related to the issue. If those updates aren't necessary for the flexGrow and widths updates being made here, I might suggest pulling those out in favor of a separate issue (if they're more enhancements you feel would be good for the codebase for example)
| 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 | ||
| }), | ||
| {} | ||
| ) |
There was a problem hiding this comment.
Similar to above, we should be able to just use our formatBreakpointMods util function instead
There was a problem hiding this comment.
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)
| @@ -0,0 +1,40 @@ | |||
| import { useState } from 'react'; | |||
There was a problem hiding this comment.
Was there an intention to add Cypress tests for this demo? Not entirely sure we need one to test the new props, so my feeling is we could remove this demo file and revert the changes made to the ToolbarDemo/ToolbarDemo.tsx file.
| 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}`, () => { |
There was a problem hiding this comment.
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 describe('ToolbarGroup' ... block, then:
- Move the flexGrow and widths tests inside that block
- remove the nested
describeblocks being used here - instead, just run a
forEachover the breakpoints array and call the tests in this forEach block
We do something similar in some of our unit test files, such as our Button tests:
… flexGrow via formatBreakpointMods; update ToolbarItem tests; remove extra demo
… flexGrow via formatBreakpointMods; update JSDoc and tests
|
Pushed updates to fix review feedback:
No extra demo files included. |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Couple more change requests below. Also looks like there's still some unrelated file updates being made:
- .yarnrc.yml should be reverted
- cleanup.js should be removed
- eslint.config.mjs should be reverted
- packager.json should be reverted
- ToolbarDemo.tsx should be reverted
- tsconfig.base.json should be reverted
- build-single-packages.mjs should be reverted
- global.d.ts should be removed
The updates/additions of those files shouldn't be necessary for this PR
| style={{ | ||
| ...style, | ||
| ...(widths | ||
| ? setBreakpointCssVars( | ||
| Object.entries(widths).reduce( | ||
| (acc, [bp, size]) => { | ||
| if (!size) { | ||
| return acc; | ||
| } | ||
| const cssVarValueMap: Record<string, string> = { | ||
| sm: 'var(--pf-c-toolbar__item--m-w-sm--Width)', | ||
| md: 'var(--pf-c-toolbar__item--m-w-md--Width)', | ||
| lg: 'var(--pf-c-toolbar__item--m-w-lg--Width)', | ||
| xl: 'var(--pf-c-toolbar__item--m-w-xl--Width)', | ||
| '2xl': 'var(--pf-c-toolbar__item--m-w-2xl--Width)', | ||
| '3xl': 'var(--pf-c-toolbar__item--m-w-3xl--Width)', | ||
| '4xl': 'var(--pf-c-toolbar__item--m-w-4xl--Width)' | ||
| }; | ||
| const value = cssVarValueMap[size as keyof typeof cssVarValueMap]; | ||
| return value ? { ...acc, [bp]: value } : acc; | ||
| }, | ||
| {} as Record<string, string> | ||
| ), | ||
| (c_toolbar__item_Width as any).name | ||
| ) | ||
| : undefined) | ||
| }} |
There was a problem hiding this comment.
Related to the above comment, we'll need to tweak this and we can simplify it a bit :
| style={{ | |
| ...style, | |
| ...(widths | |
| ? setBreakpointCssVars( | |
| Object.entries(widths).reduce( | |
| (acc, [bp, size]) => { | |
| if (!size) { | |
| return acc; | |
| } | |
| const cssVarValueMap: Record<string, string> = { | |
| sm: 'var(--pf-c-toolbar__item--m-w-sm--Width)', | |
| md: 'var(--pf-c-toolbar__item--m-w-md--Width)', | |
| lg: 'var(--pf-c-toolbar__item--m-w-lg--Width)', | |
| xl: 'var(--pf-c-toolbar__item--m-w-xl--Width)', | |
| '2xl': 'var(--pf-c-toolbar__item--m-w-2xl--Width)', | |
| '3xl': 'var(--pf-c-toolbar__item--m-w-3xl--Width)', | |
| '4xl': 'var(--pf-c-toolbar__item--m-w-4xl--Width)' | |
| }; | |
| const value = cssVarValueMap[size as keyof typeof cssVarValueMap]; | |
| return value ? { ...acc, [bp]: value } : acc; | |
| }, | |
| {} as Record<string, string> | |
| ), | |
| (c_toolbar__item_Width as any).name | |
| ) | |
| : undefined) | |
| }} | |
| style={{ | |
| ...style, | |
| ...(widths ? setBreakpointCssVars(widths, c_toolbar__item_Width.name) : undefined) | |
| }} |
Out of curiosity, where did the --pf-c-toolbar__item--m-w-xl--Width etc strings come from?
| 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'; |
There was a problem hiding this comment.
For this prop, we want the types to be just string, e.g. default?: string, as the expectation is that a consumer would pass a unit value for this prop like widths={{sm: '3rem'}} rather than us setting a token.
| 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; |
|
@khadesamrudhi it doesn't look like the previous comments have been addressed, can you confirm if you have the changes locally but haven't pushed them yet? |
Apologies for the oversight on my part. I'll be working on this and will updated you soon with the channges. Thanks for pointing it out and for your patience. |
WalkthroughAdds responsive width and flexGrow props to Toolbar components, introduces global TypeScript module declarations, updates ESLint and Yarn/build configuration, adds a cleanup script, normalizes build paths for Windows, and adds tests and a demo usage for the new Toolbar props. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-core/src/components/Toolbar/__tests__/ToolbarItem.test.tsx (1)
15-15: Typo in describe block name: "ToobarItem" should be "ToolbarItem".🔤 Suggested fix
- describe('ToobarItem rowWrap', () => { + describe('ToolbarItem rowWrap', () => {
🤖 Fix all issues with AI agents
In `@packages/react-core/src/components/Toolbar/__tests__/ToolbarGroup.test.tsx`:
- Line 30: The describe block title contains a typo; update the test suite name
in the describe call in ToolbarGroup.test.tsx from "ToobarGroup rowWrap" to
"ToolbarGroup rowWrap" so the test description accurately references the
ToolbarGroup component (locate the describe(...) string in the file and correct
the spelling).
In `@types/global.d.ts`:
- Around line 34-37: The module declaration for
'@patternfly/react-tokens/dist/esm/*' incorrectly types token exports as plain
strings; update it to export the actual token object shape used across the
codebase (e.g., an interface with properties name: string; value: string |
number; var: string) and change the exported symbol (currently tokenValue) to
that type so imports in components like ToolbarItem.tsx no longer require `as
any` casts; specifically modify the declaration in types/global.d.ts to export
the correct token interface/type for all matching modules.
- Around line 23-32: The declaration for module
'@patternfly/react-icons/dist/esm/icons/*-icon' is incorrect: replace the
FunctionComponent type and custom size prop with the correct ComponentClass
using PatternFly's SVGIconProps; update the import to use ComponentClass from
'react' and SVGIconProps from '@patternfly/react-icons', change the exported
Icon symbol to be typed as ComponentClass<SVGIconProps>, and remove the size
property from the prop shape so icons accept the standard SVG/HTML props plus
title and className.
♻️ Duplicate comments (1)
packages/react-core/src/components/Toolbar/ToolbarItem.tsx (1)
21-29: Widths prop type may conflict with reviewer feedback.The past review comment from thatblindgeye indicated that the
widthsprop types should bestring(e.g.,default?: string) so consumers can pass unit values likewidths={{ sm: '3rem' }}directly. The current implementation uses predefined size tokens ('sm' | 'md' | 'lg'etc.) that map to internal CSS variables.If the intent is to allow arbitrary width values, the types should be changed to
string. If the intent is to constrain to predefined sizes, please confirm this with the reviewer.
🧹 Nitpick comments (6)
cleanup.js (2)
11-15: Silent error suppression may mask legitimate failures.Catching and ignoring all errors without any logging makes debugging difficult when cleanup fails unexpectedly (e.g., due to file locks, permissions, or EBUSY errors). Consider at least logging at a debug level or providing a
--verboseflag.
4-24: Replacefs.rmdirSyncwithfs.rmSyncto use modern Node.js API.
fs.rmdirSyncis deprecated (DEP0147). The entiredeleteFolderRecursivefunction can be simplified usingfs.rmSyncwith{ recursive: true, force: true }, which handles all deletion in a single call:function deleteFolderRecursive(pathToDelete) { try { fs.rmSync(pathToDelete, { recursive: true, force: true }); } catch (err) { // Silently ignore errors (e.g., permission issues) } }The
force: trueoption eliminates the need for thefs.existsSynccheck, and therecursive: trueoption handles nested files and directories without manual recursion.eslint.config.mjs (1)
24-41: Defensive extraction from prettier config is reasonable but verbose.The IIFE safely extracts only flat-config-compatible keys from the prettier plugin's recommended config. While the nested try/catch and null checks are verbose, they provide resilience against plugin changes.
Consider simplifying slightly if the
cfg && typeof cfg === 'object'guard is sufficient:♻️ Minor simplification
const prettierRecommended = (() => { - try { - const cfg = prettier && prettier.configs && prettier.configs.recommended; - if (cfg && typeof cfg === 'object') { - return { - rules: cfg.rules || {}, - settings: cfg.settings || {}, - languageOptions: cfg.languageOptions || {} - }; - } - } catch (e) { - // swallow — we'll just not include it - } - return null; + const cfg = prettier?.configs?.recommended; + if (cfg && typeof cfg === 'object') { + return { + rules: cfg.rules || {}, + settings: cfg.settings || {}, + languageOptions: cfg.languageOptions || {} + }; + } + return null; })();packages/react-core/src/components/Toolbar/ToolbarItem.tsx (1)
250-276: Consider simplifying the style computation and removing theas anycast.The
(c_toolbar__item_Width as any).namecast on line 273 suggests a type mismatch. The token import should provide a typed object with anameproperty. If the type definition is incomplete, consider augmenting it rather than usingas any.Additionally, the
cssVarValueMapis recreated on every render. Consider hoisting it outside the component for minor performance optimization.♻️ Suggested improvement
+const cssVarValueMap: Record<string, string> = { + sm: 'var(--pf-c-toolbar__item--m-w-sm--Width)', + md: 'var(--pf-c-toolbar__item--m-w-md--Width)', + lg: 'var(--pf-c-toolbar__item--m-w-lg--Width)', + xl: 'var(--pf-c-toolbar__item--m-w-xl--Width)', + '2xl': 'var(--pf-c-toolbar__item--m-w-2xl--Width)', + '3xl': 'var(--pf-c-toolbar__item--m-w-3xl--Width)', + '4xl': 'var(--pf-c-toolbar__item--m-w-4xl--Width)' +}; + export const ToolbarItem: React.FunctionComponent<ToolbarItemProps> = ({Then use
cssVarValueMapdirectly in the style computation.packages/react-core/src/components/Toolbar/__tests__/ToolbarGroup.test.tsx (1)
14-28: Test structure follows existing patterns but duplicatesbpsarray.The flexGrow tests follow the same
describe.eachpattern as the existingrowWraptests, which maintains consistency within this file. Note that thebpsarray is declared twice (lines 15 and 31). Consider extracting it to a shared constant at the top of the describe block.♻️ Suggested improvement
describe('ToolbarGroup', () => { + const bps = ['default', 'sm', 'md', 'lg', 'xl', '2xl']; + it('should render with pf-m-overflow-container when isOverflowContainer is set', () => { // ... }); describe('ToolbarGroup flexGrow', () => { - const bps = ['default', 'sm', 'md', 'lg', 'xl', '2xl']; - describe.each(bps)('flexGrow at various breakpoints', (bp) => { // ... }); }); describe('ToobarGroup rowWrap', () => { - const bps = ['default', 'sm', 'md', 'lg', 'xl', '2xl']; - describe.each(bps)(`rowWrap at various breakpoints`, (bp) => { // ... }); }); });scripts/build-single-packages.mjs (1)
57-61: Inconsistent use ofcomponentvscompin path calculations.Line 59 uses the original
componentvariable while lines 60-61 use the normalizedcomp. Althoughpath.normalizeshould handle backslashes, usingcompconsistently would be clearer and more predictable.♻️ Suggested fix for consistency
const pathAsArray = comp.split('/'); const destDir = path.normalize(destFile.replace(/package\.json$/, '')); - const esmRelative = path.relative(destDir, path.normalize(component)); + const esmRelative = path.relative(destDir, path.normalize(comp)); const cjsRelative = path.relative(destDir, path.normalize(comp.replace('/dist/esm/', '/dist/js/'))); const typesRelative = path.relative(destDir, path.normalize(comp.replace(/\.js$/, '.d.ts')));
| }); | ||
| }); | ||
|
|
||
| describe('ToobarGroup rowWrap', () => { |
There was a problem hiding this comment.
Typo in describe block name: "ToobarGroup" should be "ToolbarGroup".
🔤 Suggested fix
- describe('ToobarGroup rowWrap', () => {
+ describe('ToolbarGroup rowWrap', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('ToobarGroup rowWrap', () => { | |
| describe('ToolbarGroup rowWrap', () => { |
🤖 Prompt for AI Agents
In `@packages/react-core/src/components/Toolbar/__tests__/ToolbarGroup.test.tsx`
at line 30, The describe block title contains a typo; update the test suite name
in the describe call in ToolbarGroup.test.tsx from "ToobarGroup rowWrap" to
"ToolbarGroup rowWrap" so the test description accurately references the
ToolbarGroup component (locate the describe(...) string in the file and correct
the spelling).
| declare module '@patternfly/react-icons/dist/esm/icons/*-icon' { | ||
| import type { FunctionComponent, SVGProps } from 'react'; | ||
| const Icon: FunctionComponent< | ||
| SVGProps<SVGSVGElement> & { | ||
| title?: string; | ||
| size?: 'sm' | 'md' | 'lg' | 'xl'; | ||
| } | ||
| >; | ||
| export default Icon; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if PatternFly is in the repository
find . -name "package.json" -type f -not -path "*/node_modules/*" | head -5Repository: patternfly/patternfly-react
Length of output: 274
🏁 Script executed:
# Check for PatternFly icons in dependencies
grep -r "patternfly/react-icons" . --include="package.json" 2>/dev/null | head -10Repository: patternfly/patternfly-react
Length of output: 647
🏁 Script executed:
# Look for icon files in the project
fd -t f "icon" --extension ts --extension tsx --extension d.ts | grep -i patternfly | head -20Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Search for any imports or references to PatternFly icons in the codebase
rg "@patternfly/react-icons" --type ts --type tsx -l | head -20Repository: patternfly/patternfly-react
Length of output: 98
🏁 Script executed:
# List contents of react-icons package
ls -la ./packages/react-icons/Repository: patternfly/patternfly-react
Length of output: 826
🏁 Script executed:
# Find icon component files
find ./packages/react-icons -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.d.ts" \) | head -20Repository: patternfly/patternfly-react
Length of output: 205
🏁 Script executed:
# Check for icon definitions in the package
find ./packages/react-icons -name "*icon*" -type f | head -30Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Look for an example icon file to examine its structure
fd "Icon" ./packages/react-icons --type f --extension ts --extension js | head -10Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Examine the createIcon function which likely defines the icon component interface
cat -n ./packages/react-icons/src/createIcon.tsxRepository: patternfly/patternfly-react
Length of output: 2569
🏁 Script executed:
# Check the main index file
cat -n ./packages/react-icons/src/index.tsRepository: patternfly/patternfly-react
Length of output: 108
🏁 Script executed:
# Check if there are type definitions
find ./packages/react-icons -name "*.d.ts" -type fRepository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Look for dist folders or built icons
find ./packages/react-icons -name "dist" -o -name "esm" -type dRepository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Check for actual icon files (they might be generated)
find ./packages/react-icons/src -type f -name "*-icon*" | head -10Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Look for icons directory
ls -la ./packages/react-icons/src/icons/ 2>/dev/null | head -20Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Check the build/scripts to see how icons are generated
cat ./packages/react-icons/scripts/build-icons.js 2>/dev/null || cat ./packages/react-icons/scripts/*.js 2>/dev/null | head -50Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Check package.json in react-icons for build scripts
cat ./packages/react-icons/package.jsonRepository: patternfly/patternfly-react
Length of output: 1534
🏁 Script executed:
# Search more broadly for icon files
find ./packages/react-icons -type f | head -30Repository: patternfly/patternfly-react
Length of output: 1037
🏁 Script executed:
# Check if there's a build output or if icons come from elsewhere
ls -la ./packages/react-icons/src/Repository: patternfly/patternfly-react
Length of output: 362
🏁 Script executed:
# Look at the README for documentation
cat ./packages/react-icons/README.mdRepository: patternfly/patternfly-react
Length of output: 3037
🏁 Script executed:
# Check the writeIcons script to see what types are generated
head -100 ./packages/react-icons/scripts/writeIcons.mjsRepository: patternfly/patternfly-react
Length of output: 3243
Remove size prop and correct component type to ComponentClass.
The type declaration is inaccurate. Generated icons are ComponentClass<SVGIconProps>, not FunctionComponent, and they don't support a size prop. The correct declaration should be:
declare module '@patternfly/react-icons/dist/esm/icons/*-icon' {
import type { ComponentClass } from 'react';
import type { SVGIconProps } from '@patternfly/react-icons';
const Icon: ComponentClass<SVGIconProps>;
export default Icon;
}Icons support all HTMLProps<SVGElement> attributes (except ref) plus title and className.
🤖 Prompt for AI Agents
In `@types/global.d.ts` around lines 23 - 32, The declaration for module
'@patternfly/react-icons/dist/esm/icons/*-icon' is incorrect: replace the
FunctionComponent type and custom size prop with the correct ComponentClass
using PatternFly's SVGIconProps; update the import to use ComponentClass from
'react' and SVGIconProps from '@patternfly/react-icons', change the exported
Icon symbol to be typed as ComponentClass<SVGIconProps>, and remove the size
property from the prop shape so icons accept the standard SVG/HTML props plus
title and className.
| declare module '@patternfly/react-tokens/dist/esm/*' { | ||
| const tokenValue: string; | ||
| export default tokenValue; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if node_modules exists and examine token export
ls -la node_modules/@patternfly/react-tokens/dist/esm/ 2>/dev/null | head -20Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Look for token files in the repository (they might be packaged or referenced)
find . -type f -name "*c_toolbar__item_Width*" 2>/dev/null | head -10Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Check the current global.d.ts file to see the exact type declaration
cat -n types/global.d.tsRepository: patternfly/patternfly-react
Length of output: 1511
🏁 Script executed:
# Search for ToolbarItem.tsx and the as any cast mentioned
find . -type f -name "ToolbarItem.tsx" 2>/dev/null | head -5Repository: patternfly/patternfly-react
Length of output: 131
🏁 Script executed:
# Search for 'as any' usage related to tokens in the codebase
rg "as any" --type typescript --type tsx -B 2 -A 2Repository: patternfly/patternfly-react
Length of output: 105
🏁 Script executed:
cat -n packages/react-core/src/components/Toolbar/ToolbarItem.tsxRepository: patternfly/patternfly-react
Length of output: 11722
🏁 Script executed:
# Search for token imports and 'as any' usage in tsx/ts files
rg "as any" -A 2 -B 2 --glob "*.tsx" --glob "*.ts"Repository: patternfly/patternfly-react
Length of output: 50384
🏁 Script executed:
# Search for `@patternfly/react-tokens` imports in the codebase
rg "@patternfly/react-tokens" --glob "*.tsx" --glob "*.ts"Repository: patternfly/patternfly-react
Length of output: 50384
🏁 Script executed:
# Try to understand the actual structure of react-tokens by searching package info or types
rg "tokenValue|token.*name.*value.*var" --glob "*.d.ts" --glob "*.ts"Repository: patternfly/patternfly-react
Length of output: 329
Fix the token type declaration to match the actual token object structure.
The @patternfly/react-tokens exports objects with { name: string; value: string | number; var: string } properties, not plain strings. The current type declaration is incorrect, which is why as any casts are needed in ToolbarItem.tsx and elsewhere.
🔧 Suggested fix
declare module '@patternfly/react-tokens/dist/esm/*' {
- const tokenValue: string;
- export default tokenValue;
+ interface Token {
+ name: string;
+ value: string | number;
+ var: string;
+ }
+ const token: Token;
+ export default token;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| declare module '@patternfly/react-tokens/dist/esm/*' { | |
| const tokenValue: string; | |
| export default tokenValue; | |
| } | |
| declare module '@patternfly/react-tokens/dist/esm/*' { | |
| interface Token { | |
| name: string; | |
| value: string | number; | |
| var: string; | |
| } | |
| const token: Token; | |
| export default token; | |
| } |
🤖 Prompt for AI Agents
In `@types/global.d.ts` around lines 34 - 37, The module declaration for
'@patternfly/react-tokens/dist/esm/*' incorrectly types token exports as plain
strings; update it to export the actual token object shape used across the
codebase (e.g., an interface with properties name: string; value: string |
number; var: string) and change the exported symbol (currently tokenValue) to
that type so imports in components like ToolbarItem.tsx no longer require `as
any` casts; specifically modify the declaration in types/global.d.ts to export
the correct token interface/type for all matching modules.
|
Hi @thatblindgeye, @mcoker, @kmcfaul All requested changes have been applied:
Please review again. Thanks! |
3 participants
This PR adds the previously removed
widthsprop to<ToolbarItem />and introduces a newflexGrowprop that maps to.pf-m-flex-grow[on-{breakpoint}]modifiers.Changes
widthsandflexGrowsupport usingformatBreakpointModsCloses #11910
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.