-
Notifications
You must be signed in to change notification settings - Fork 42
✨ Enhance pf6 rules #347
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?
✨ Enhance pf6 rules #347
Conversation
📝 WalkthroughWalkthroughThis PR updates and consolidates PatternFly v5-to-v6 migration rules across 30+ YAML configuration files. Changes include revised rule descriptions, expanded migration messages with detailed before/after code examples, updated regex patterns for improved detection, addition of new semantic token migration rules, removal of deprecated pattern rules, and reorganization of component API migration guidance to reflect breaking changes in properties, naming, and component structure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
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.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-component-renames.yaml (1)
84-120: Tighten the match to avoid false positives after migration.The rule only checks for the import of
MastheadBrand, but v6-migrated code still imports it (now as a wrapper forMastheadLogo). This causes the rule to trigger on already-migrated code. Match the old usage pattern instead: require both the import AND the old<MastheadBrand href="...">structure, which is absent in v6-style code whereMastheadLogoholds thehref.🔍 Suggested pattern refinement
when: + and: - - builtin.filecontent: + - builtin.filecontent: pattern: import[\s\S]*MastheadBrand[\s\S]*from[\s\S]*['"]@patternfly/react-core['"] filePattern: \.(j|t)sx?$ + - builtin.filecontent: + pattern: <MastheadBrand[^>]*\bhref\s*= + filePattern: \.(j|t)sx?$
🤖 Fix all issues with AI agents
In `@preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-components.yaml`:
- Around line 145-148: The current builtin.filecontent pattern <Modal[^>]*> is
too broad and flags all Modal usages; update the YAML rule in
patternfly-v5-to-patternfly-v6-components.yaml to target deprecated Modal props
instead by changing the regex in the builtin.filecontent pattern to match Modal
elements that include deprecated prop names (e.g., match attributes like title=,
actions=, or header=) so the rule only flags Modal instances using those
deprecated APIs (look for the builtin.filecontent entry and its pattern value to
modify).
In `@preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-css-variables.yaml`:
- Around line 9-25: The rule currently only matches --pf-v5-global--FontSize--lg
but the guidance lists many sizes; update the file match pattern
(builtin.filecontent.pattern) to cover all FontSize tokens (e.g. change
--pf-v5-global--FontSize--lg to a regex like
--pf-v5-global--FontSize--(xs|sm|md|lg|xl|2xl|3xl|4xl) or
--pf-v5-global--FontSize--[a-z0-9-]+) and optionally adjust the message to make
it clear the rule applies to every matched size and maps each to its PatternFly
6 token equivalents (references: builtin.filecontent.pattern and the documented
tokens such as --pf-t--global--font--size--body--lg and the other mapping
entries).
In `@preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-masthead.yaml`:
- Around line 42-44: The link entry under the links array uses a misleading
title; update the title value (the "title" field of the link object) to reflect
the actual GitHub pull request target—for example change "patternfly-v6
Documentation" to "PatternFly PR: Masthead component changes" or similar
descriptive text so the title matches the URL
(https://github.com/patternfly/patternfly-react/pull/10809).
- Around line 87-89: The links array contains an entry whose title
("patternfly-v6 Documentation") does not match the URL target (a GitHub PR
https://github.com/patternfly/patternfly-react/pull/9774); update the title
field for that entry in the links list to accurately describe the target (e.g.,
include "GitHub PR `#9774`" or "patternfly-react PR `#9774`") so the title and url
are consistent—edit the title value for the link item shown in the links block.
In `@preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-pagesection.yaml`:
- Around line 107-109: The regex currently places the negative lookahead after a
greedy [^>]* so it always succeeds; change the pattern so the negative lookahead
for the variant prop is applied immediately after the `<PageSection` token
(i.e., use an immediate `(?![^>]*\bvariant\b)` right after `<PageSection`) and
then continue to match the rest of the tag for `hasBodyWrapper={false}` with
your existing `[^>]*` segments; update the rule that references `PageSection` /
`hasBodyWrapper` / `variant` accordingly so tags containing a `variant` prop are
excluded.
- Around line 140-186: The rule patternfly-v5-to-patternfly-v6-pagesection-00030
incorrectly asserts PageSection's isFilled prop was removed; update the rule to
stop flagging <PageSection ... isFilled> usage (remove the builtin.filecontent
check for \bisFilled\b or disable the rule entirely) and revise the
description/message to either (a) note that isFilled remains supported in
PatternFly React v6 or (b) present the pf-m-fill/class or style={{flex:'1 1
auto'}} options as optional alternatives; ensure references to PageSection and
isFilled in the rule metadata and message are updated to reflect the correct
documentation.
🧹 Nitpick comments (10)
preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-drawer.yaml (1)
58-83: Consider aligning message formatting with rule 00000.Rules 00010-00040 use quoted string syntax with escaped quotes (
''light-200'') and embedded newlines, while rule 00000 now uses the cleaner YAML multiline block scalar (|) with proper markdown code blocks. For consistency and improved readability, consider updating these rules to match the formatting style used in rule 00000.preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-chip.yaml (2)
2-2: Minor: Consider clarifying the description.The current phrasing "Chip import should be replaced with Chip import from deprecated" is grammatically incomplete. Consider making it more explicit.
Suggested improvement
-- description: Chip import should be replaced with Chip import from deprecated +- description: Chip import from `@patternfly/react-core` should be updated to use the deprecated path
46-46: Minor: Same grammar issue as above.Same suggestion applies here for consistency.
Suggested improvement
-- description: ChipGroup import should be replaced with ChipGroup import from deprecated +- description: ChipGroup import from `@patternfly/react-core` should be updated to use the deprecated pathpreview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-menu.yaml (2)
11-38: Migration guidance is clear and helpful.The message effectively documents both the import changes and the component usage changes, including the prop rename from
onToggletoonClick. Thearia-hidden="true"on the icon is a good accessibility practice.Minor nit: The "Import changes" header uses bold markdown formatting while "Before:" and "After:" do not. Consider making the formatting consistent across all section headers.
✨ Optional: Consistent header formatting
**Import changes:** ```tsx // Before import { KebabToggle } from '@patternfly/react-core'; // After import { MenuToggle } from '@patternfly/react-core'; import { EllipsisVIcon } from '@patternfly/react-icons'; ``` - Before: + **Before:** ```tsx <KebabToggle onToggle={() => {}} /> ``` - After: + **After:** ```tsx <MenuToggle
39-41: Link title doesn't match the actual resource.The link title says "patternfly-v6 Documentation" but the URL points to a GitHub pull request (
/pull/10345). Consider updating the title to accurately reflect the linked resource (e.g., "Related PatternFly PR") or linking to official PatternFly v6 migration documentation if available.✨ Suggested title update
links: - url: https://github.com/patternfly/patternfly-react/pull/10345 - title: patternfly-v6 Documentation + title: PatternFly PR - KebabToggle removalpreview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-deprecated-components.yaml (1)
44-85: Guidance is accurate; consider adding installation instructions for clarity.The guidance correctly identifies
DragDropSortfrom@patternfly/react-drag-dropas the recommended replacement forDragDrop/Draggable/Droppablein PatternFly v6, and the package name is correct. To make the migration more actionable, consider adding a note that developers need to install the new package:npm i `@patternfly/react-drag-drop`This is built on dnd-kit and is the officially supported path forward per PatternFly v6 documentation.
Also applies to: 87-128, 130-171
preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-removed-props.yaml (1)
322-327: Loosen enum-prop regex to allow whitespace.
type=\{PageSectionTypes\.nav\}won’t match common formatting liketype = { PageSectionTypes.nav }. Allowing optional whitespace makes the rule more resilient.♻️ Suggested regex tweak
- pattern: <PageSection[^>]*\btype=\{PageSectionTypes\.nav\} + pattern: <PageSection[^>]*\btype\s*=\s*\{\s*PageSectionTypes\.nav\s*\}preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-spacing-tokens.yaml (1)
1-395: Consider renaming or splitting this file to match token types.
It contains spacing, font, and border rules, so “spacing-tokens” may mislead future maintainers.preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-masthead.yaml (1)
9-10: Pattern may trigger false positives on already-migrated code.The pattern
MastheadBrandwill match both the old v5 component (that needs migration) and the new v6MastheadBrandwrapper component. After migration, this rule will continue to fire on correctly migrated code.Consider refining the pattern to detect v5 usage specifically, such as checking for
MastheadBrandwithout an adjacentMastheadLogochild, or accept this as a known limitation given the complexity of distinguishing the two cases.preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-component-props.yaml (1)
1156-1162: Consider anchoringalignRightto alignment props to reduce noise.Line 1160 matches any
alignRighttoken in files that reference any of the listed components. If this is noisy, tighten the regex to align/ alignment props and mirror the change foralignLeft.♻️ Suggested regex tightening
- pattern: \balignRight\b + pattern: \b(?:align|alignment)\s*=\s*(?:\{[^}]*\balignRight\b|["']alignRight["'])
| pattern: Modal | ||
| - builtin.filecontent: | ||
| pattern: <Button[^>]*\bisActive\b | ||
| pattern: <Modal[^>]*> | ||
| filePattern: \.(j|t)sx?$ |
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.
Regex pattern is too broad and will cause false positives.
The pattern <Modal[^>]*> matches any Modal usage, including valid v6 code that already uses the composed ModalHeader/ModalBody/ModalFooter structure. This will flag components that have already been migrated.
Consider targeting specific deprecated props to avoid false positives:
🔧 Suggested fix to target deprecated API usage
- builtin.filecontent:
- pattern: <Modal[^>]*>
+ pattern: <Modal[^>]*\b(title|actions|header)=
filePattern: \.(j|t)sx?$This targets Modals using the deprecated title, actions, or header props rather than all Modal instances.
📝 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.
| pattern: Modal | |
| - builtin.filecontent: | |
| pattern: <Button[^>]*\bisActive\b | |
| pattern: <Modal[^>]*> | |
| filePattern: \.(j|t)sx?$ | |
| pattern: Modal | |
| - builtin.filecontent: | |
| pattern: <Modal[^>]*\b(title|actions|header)= | |
| filePattern: \.(j|t)sx?$ |
🤖 Prompt for AI Agents
In `@preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-components.yaml`
around lines 145 - 148, The current builtin.filecontent pattern <Modal[^>]*> is
too broad and flags all Modal usages; update the YAML rule in
patternfly-v5-to-patternfly-v6-components.yaml to target deprecated Modal props
instead by changing the regex in the builtin.filecontent pattern to match Modal
elements that include deprecated prop names (e.g., match attributes like title=,
actions=, or header=) so the rule only flags Modal instances using those
deprecated APIs (look for the builtin.filecontent entry and its pattern value to
modify).
| builtin.filecontent: | ||
| pattern: --pf-v5-global--FontSize--lg | ||
| filePattern: \.(css|scss)$ | ||
| message: 'CSS variable naming has changed in PatternFly 6 with the new design token | ||
|
|
||
| system | ||
|
|
||
|
|
||
|
|
||
| Replace `--pf-v5-global--FontSize--lg` with `--pf-t--global--font--size--lg`. | ||
|
|
||
|
|
||
| filePattern: \.(css|scss|js|jsx|ts|tsx)$ | ||
| message: | | ||
| CSS variable naming has changed in PatternFly 6 with the new design token system. | ||
|
|
||
| Replace `--pf-v5-global--FontSize--lg` with `--pf-t--global--font--size--body--lg`. | ||
|
|
||
| Common font size mappings: | ||
| - `--pf-v5-global--FontSize--xs` → `--pf-t--global--font--size--body--sm` | ||
| - `--pf-v5-global--FontSize--sm` → `--pf-t--global--font--size--body--sm` | ||
| - `--pf-v5-global--FontSize--md` → `--pf-t--global--font--size--body--default` | ||
| - `--pf-v5-global--FontSize--lg` → `--pf-t--global--font--size--body--lg` | ||
| - `--pf-v5-global--FontSize--xl` → `--pf-t--global--font--size--heading--md` | ||
| - `--pf-v5-global--FontSize--2xl` → `--pf-t--global--font--size--heading--lg` | ||
| - `--pf-v5-global--FontSize--3xl` → `--pf-t--global--font--size--heading--xl` | ||
| - `--pf-v5-global--FontSize--4xl` → `--pf-t--global--font--size--heading--h1` |
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.
Rule only triggers on FontSize--lg, but the message documents multiple sizes.
As written, this won’t fire for xs/sm/md/xl/2xl/3xl/4xl unless a file also contains --lg. Consider broadening the match (or adding separate rules) so the guidance is surfaced whenever any FontSize token appears.
✅ Suggested update to broaden the match
- pattern: --pf-v5-global--FontSize--lg
+ pattern: --pf-v5-global--FontSize--📝 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.
| builtin.filecontent: | |
| pattern: --pf-v5-global--FontSize--lg | |
| filePattern: \.(css|scss)$ | |
| message: 'CSS variable naming has changed in PatternFly 6 with the new design token | |
| system | |
| Replace `--pf-v5-global--FontSize--lg` with `--pf-t--global--font--size--lg`. | |
| filePattern: \.(css|scss|js|jsx|ts|tsx)$ | |
| message: | | |
| CSS variable naming has changed in PatternFly 6 with the new design token system. | |
| Replace `--pf-v5-global--FontSize--lg` with `--pf-t--global--font--size--body--lg`. | |
| Common font size mappings: | |
| - `--pf-v5-global--FontSize--xs` → `--pf-t--global--font--size--body--sm` | |
| - `--pf-v5-global--FontSize--sm` → `--pf-t--global--font--size--body--sm` | |
| - `--pf-v5-global--FontSize--md` → `--pf-t--global--font--size--body--default` | |
| - `--pf-v5-global--FontSize--lg` → `--pf-t--global--font--size--body--lg` | |
| - `--pf-v5-global--FontSize--xl` → `--pf-t--global--font--size--heading--md` | |
| - `--pf-v5-global--FontSize--2xl` → `--pf-t--global--font--size--heading--lg` | |
| - `--pf-v5-global--FontSize--3xl` → `--pf-t--global--font--size--heading--xl` | |
| - `--pf-v5-global--FontSize--4xl` → `--pf-t--global--font--size--heading--h1` | |
| builtin.filecontent: | |
| pattern: --pf-v5-global--FontSize-- | |
| filePattern: \.(css|scss|js|jsx|ts|tsx)$ | |
| message: | | |
| CSS variable naming has changed in PatternFly 6 with the new design token system. | |
| Replace `--pf-v5-global--FontSize--lg` with `--pf-t--global--font--size--body--lg`. | |
| Common font size mappings: | |
| - `--pf-v5-global--FontSize--xs` → `--pf-t--global--font--size--body--sm` | |
| - `--pf-v5-global--FontSize--sm` → `--pf-t--global--font--size--body--sm` | |
| - `--pf-v5-global--FontSize--md` → `--pf-t--global--font--size--body--default` | |
| - `--pf-v5-global--FontSize--lg` → `--pf-t--global--font--size--body--lg` | |
| - `--pf-v5-global--FontSize--xl` → `--pf-t--global--font--size--heading--md` | |
| - `--pf-v5-global--FontSize--2xl` → `--pf-t--global--font--size--heading--lg` | |
| - `--pf-v5-global--FontSize--3xl` → `--pf-t--global--font--size--heading--xl` | |
| - `--pf-v5-global--FontSize--4xl` → `--pf-t--global--font--size--heading--h1` |
🤖 Prompt for AI Agents
In `@preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-css-variables.yaml`
around lines 9 - 25, The rule currently only matches
--pf-v5-global--FontSize--lg but the guidance lists many sizes; update the file
match pattern (builtin.filecontent.pattern) to cover all FontSize tokens (e.g.
change --pf-v5-global--FontSize--lg to a regex like
--pf-v5-global--FontSize--(xs|sm|md|lg|xl|2xl|3xl|4xl) or
--pf-v5-global--FontSize--[a-z0-9-]+) and optionally adjust the message to make
it clear the rule applies to every matched size and maps each to its PatternFly
6 token equivalents (references: builtin.filecontent.pattern and the documented
tokens such as --pf-t--global--font--size--body--lg and the other mapping
entries).
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.
good catch. @pranavgaikwad Look at this
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.
@pranavgaikwad Great point! You're right to flag this. The current rule only triggers when --pf-v5-global--FontSize--lg is present, but the message documents mappings for all 8 size variants (xs, sm, md, lg, xl, 2xl, 3xl, 4xl). This means the helpful guidance won't appear for developers using the other 7 sizes unless their file also happens to contain the lg variant.
Would you like me to update the pattern to match all FontSize tokens? I can broaden it to --pf-v5-global--FontSize-- (or use a regex like --pf-v5-global--FontSize--(xs|sm|md|lg|xl|2xl|3xl|4xl)) so the rule fires for any FontSize token. This way, developers will see the comprehensive mapping guidance regardless of which size they're using.
Let me know if you'd like me to implement this change! 🚀
🧠 Learnings used
Learnt from: abrugaro
Repo: konveyor/rulesets PR: 331
File: preview/nodejs/astro/astro-v4-to-v5/astro-4-to-astro-5-css-changes.yaml:0-0
Timestamp: 2026-01-07T15:56:47.779Z
Learning: In Astro migration rules, filePattern restrictions should be avoided for CSS-related patterns because Astro allows HTML and inline styles in multiple file types including .md, .mdx, and .ts files, not just .css, .html, or .astro files.
| links: | ||
| - url: https://github.com/patternfly/patternfly-react/pull/10809 | ||
| title: patternfly-v6 Documentation |
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.
Link title is misleading.
The link points to a GitHub pull request but the title says "patternfly-v6 Documentation". Consider updating the title to accurately reflect the link target, e.g., "PatternFly PR: Masthead component changes".
Proposed fix
links:
- url: https://github.com/patternfly/patternfly-react/pull/10809
- title: patternfly-v6 Documentation
+ title: "PatternFly PR `#10809`: Masthead component changes"📝 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.
| links: | |
| - url: https://github.com/patternfly/patternfly-react/pull/10809 | |
| title: patternfly-v6 Documentation | |
| links: | |
| - url: https://github.com/patternfly/patternfly-react/pull/10809 | |
| title: "PatternFly PR `#10809`: Masthead component changes" |
🤖 Prompt for AI Agents
In `@preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-masthead.yaml`
around lines 42 - 44, The link entry under the links array uses a misleading
title; update the title value (the "title" field of the link object) to reflect
the actual GitHub pull request target—for example change "patternfly-v6
Documentation" to "PatternFly PR: Masthead component changes" or similar
descriptive text so the title matches the URL
(https://github.com/patternfly/patternfly-react/pull/10809).
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.
good catch. @pranavgaikwad take a look at this
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.
@pranavgaikwad, thanks for acknowledging! Looking forward to seeing the link title updated to better reflect that it points to a specific PR rather than general documentation. Let me know if you need any help with the changes! 🙂
| links: | ||
| - url: https://github.com/patternfly/patternfly-react/pull/9774 | ||
| title: patternfly-v6 Documentation |
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.
Link title inconsistent with actual target.
Same issue as the first rule - the link points to a GitHub PR but title says "patternfly-v6 Documentation".
Proposed fix
links:
- url: https://github.com/patternfly/patternfly-react/pull/9774
- title: patternfly-v6 Documentation
+ title: "PatternFly PR `#9774`: backgroundColor prop removal"📝 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.
| links: | |
| - url: https://github.com/patternfly/patternfly-react/pull/9774 | |
| title: patternfly-v6 Documentation | |
| links: | |
| - url: https://github.com/patternfly/patternfly-react/pull/9774 | |
| title: "PatternFly PR `#9774`: backgroundColor prop removal" |
🤖 Prompt for AI Agents
In `@preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-masthead.yaml`
around lines 87 - 89, The links array contains an entry whose title
("patternfly-v6 Documentation") does not match the URL target (a GitHub PR
https://github.com/patternfly/patternfly-react/pull/9774); update the title
field for that entry in the links list to accurately describe the target (e.g.,
include "GitHub PR `#9774`" or "patternfly-react PR `#9774`") so the title and url
are consistent—edit the title value for the link item shown in the links block.
| - builtin.filecontent: | ||
| pattern: <PageSection[^>]*\bhasBodyWrapper=\{false\}[^>]*(?!variant) | ||
| filePattern: \.(j|t)sx?$ |
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.
Regex pattern does not correctly exclude tags with variant prop.
The negative lookahead (?!variant) at the end of the pattern doesn't work as intended. The greedy [^>]* before it consumes all characters up to >, so the lookahead checks at a position where only > follows—it will always pass, matching tags even when they contain a variant prop.
For example, <PageSection hasBodyWrapper={false} variant="secondary"> will incorrectly match this rule.
🐛 Proposed fix: move negative lookahead to the start
- builtin.filecontent:
- pattern: <PageSection[^>]*\bhasBodyWrapper=\{false\}[^>]*(?!variant)
+ pattern: <PageSection(?![^>]*\bvariant\b)[^>]*\bhasBodyWrapper=\{false\}
filePattern: \.(j|t)sx?$This places the negative lookahead immediately after <PageSection, checking that variant does not appear anywhere in the tag before matching hasBodyWrapper={false}.
🤖 Prompt for AI Agents
In `@preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-pagesection.yaml`
around lines 107 - 109, The regex currently places the negative lookahead after
a greedy [^>]* so it always succeeds; change the pattern so the negative
lookahead for the variant prop is applied immediately after the `<PageSection`
token (i.e., use an immediate `(?![^>]*\bvariant\b)` right after `<PageSection`)
and then continue to match the rest of the tag for `hasBodyWrapper={false}` with
your existing `[^>]*` segments; update the rule that references `PageSection` /
`hasBodyWrapper` / `variant` accordingly so tags containing a `variant` prop are
excluded.
| - ruleID: patternfly-v5-to-patternfly-v6-pagesection-00030 | ||
| description: PageSection isFilled prop has been removed | ||
| effort: 3 | ||
| category: mandatory | ||
| labels: | ||
| - konveyor.io/source=patternfly-v5 | ||
| - konveyor.io/target=patternfly-v6 | ||
| when: | ||
| and: | ||
| - nodejs.referenced: | ||
| pattern: PageSection | ||
| - builtin.filecontent: | ||
| pattern: <PageSection[^>]*\bisFilled\b | ||
| filePattern: \.(j|t)sx?$ | ||
| message: | | ||
| The `isFilled` prop on PageSection has been removed in PatternFly 6. | ||
|
|
||
| Use CSS flex properties to control how PageSection fills available space. | ||
|
|
||
| Before: | ||
|
|
||
| ```tsx | ||
| <PageSection isFilled> | ||
| {content} | ||
| </PageSection> | ||
| ``` | ||
|
|
||
| After: | ||
|
|
||
| ```tsx | ||
| <PageSection style={{ flex: '1 1 auto' }}> | ||
| {content} | ||
| </PageSection> | ||
| ``` | ||
|
|
||
| Or use a custom class: | ||
|
|
||
| ```tsx | ||
| <PageSection className="pf-m-fill"> | ||
| {content} | ||
| </PageSection> | ||
| ``` | ||
| links: | ||
| - url: https://www.patternfly.org/components/page/ | ||
| title: PatternFly Page Component | ||
| customVariables: [] | ||
| migration_complexity: low |
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.
🧩 Analysis chain
🌐 Web query:
PatternFly v6 pf-m-fill utility class PageSection
💡 Result:
In PatternFly v6, “fill” is a modifier (.pf-m-fill) you apply to a page main section (what React’s <PageSection> renders as) so it can grow to fill the remaining vertical space.
HTML/CSS (Page section fill)
Apply .pf-m-fill to the section and ensure the page’s main container can provide height (it also needs .pf-m-fill):
<div class="pf-v6-c-page">
<main class="pf-v6-c-page__main" tabindex="-1" id="main-content">
<div class="pf-v6-c-page__main-container pf-m-fill">
<section class="pf-v6-c-page__main-section pf-m-fill">
...
</section>
</div>
</main>
</div>PatternFly notes that the .pf-v6-c-page__main-container must also have .pf-m-fill for the section to stretch to full height. [1]
React (PatternFly React v6)
Enable fill behavior on the page, then mark the specific sections/groups that should fill:
<Page isContentFilled /><PageSection isFilled />(or<PageGroup isFilled />)
This is documented under “Filled page sections.” [2]
Sources:
[1] PatternFly v6 Page (HTML) docs (fill modifier and requirement)
[2] PatternFly React v6 Page docs (“Filled page sections”)
The rule is based on incorrect information: isFilled prop still exists in PatternFly React v6.
While pf-m-fill is indeed a valid PatternFly v6 modifier class, the rule's premise is flawed. According to PatternFly React v6 documentation, the isFilled prop on <PageSection> is NOT removed—it's still available and functional. The rule should not flag this prop as a breaking change. Remove or significantly revise this rule.
🤖 Prompt for AI Agents
In `@preview/nodejs/patternfly/patternfly-v5-to-patternfly-v6-pagesection.yaml`
around lines 140 - 186, The rule
patternfly-v5-to-patternfly-v6-pagesection-00030 incorrectly asserts
PageSection's isFilled prop was removed; update the rule to stop flagging
<PageSection ... isFilled> usage (remove the builtin.filecontent check for
\bisFilled\b or disable the rule entirely) and revise the description/message to
either (a) note that isFilled remains supported in PatternFly React v6 or (b)
present the pf-m-fill/class or style={{flex:'1 1 auto'}} options as optional
alternatives; ensure references to PageSection and isFilled in the rule metadata
and message are updated to reflect the correct documentation.
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
ab1798b to
8b2839b
Compare
| - konveyor.io/target=patternfly-v6 | ||
| when: | ||
| builtin.filecontent: | ||
| pattern: isDisabled |
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.
This and the following 2 rules matched for all components. But not all components have this issue. There are only a certain components which are covered by their specific rules already
jmle
left a comment
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.
Looks good, I'm just wondering now if we shouldn't have left these rules in "preview".
|
@jmle Thanks for taking a look at this. I moved these to stable/ already. And also, there's some more changes incoming for this. |
Summary by CodeRabbit
PatternFly v5 to v6 Migration Guidance Updates
✏️ Tip: You can customize this high-level summary in your review settings.