OCPBUGS-62638: Update PatternFly to v6.4.0 #15555
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/assign @logonoff |
I remember there being some minor visual regressions in the create forms, can you check if those still exist? If there are any regressions with console or dynamic plugins I think we should defer upgrading PF to 4.22 so we can count it as a part of our "breaking" changes Also PF 6.4.0 is coming out soon so if we're updating to PF we should probably wait for that |
|
/hold for PF6.4 |
openshift-ci
bot
added
the
do-not-merge/hold
|
/retitle OCPBUGS-62638: Update PatternFly to v6.3.0+ so .co-horizontal-nav override can be removed |
openshift-ci
bot
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@rhamilto: This pull request references Jira Issue OCPBUGS-62638, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
logonoff
force-pushed
the
OCPBUGS-62638
branch
from
2cec8c0 to
f28c083
Compare
|
Spotted while trying to adopt favorites variant buttons: patternfly/patternfly#7894 Spotted while trying to adopt settings variant buttons: patternfly/patternfly-react#12065 |
logonoff
force-pushed
the
OCPBUGS-62638
branch
from
f28c083 to
f47c980
Compare
openshift-ci
bot
added
component/shared
logonoff
force-pushed
the
OCPBUGS-62638
branch
from
9e3360d to
9a4d692
Compare
|
Hold until OCP 4.22 /hold /retest OCPBUGS-62638: Update PatternFly to v6.4.0 |
|
@logonoff: The The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retitle OCPBUGS-62638: Update PatternFly to v6.4.0 |
openshift-ci
bot
changed the title
logonoff
force-pushed
the
OCPBUGS-62638
branch
from
63cdb0e to
f129893
Compare
logonoff
force-pushed
the
OCPBUGS-62638
branch
2 times, most recently
from
0f39884 to
26f81cd
Compare
|
@rhamilto this one needs rebase 😭 |
openshift-merge-robot
added
the
needs-rebase
logonoff
force-pushed
the
OCPBUGS-62638
branch
from
26f81cd to
8f16bd2
Compare
openshift-merge-robot
removed
the
needs-rebase
|
Warning Rate limit exceeded@logonoff has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (11)
WalkthroughAligned many PatternFly dependencies to v6.4.x, added/updated PatternFly packages, removed local Monaco theme registration, migrated several components to use PatternFly component props instead of local icons, removed TypeScript suppression comments on Select handlers, adjusted SCSS variables/overrides, and updated a package-check script and README note. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
frontend/package.json(1 hunks)frontend/packages/console-app/src/components/access-modes/access-mode.tsx(0 hunks)frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx(1 hunks)frontend/packages/console-app/src/components/favorite/FavoriteNavItems.tsx(1 hunks)frontend/packages/console-dynamic-plugin-sdk/README.md(1 hunks)frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx(1 hunks)frontend/packages/console-shared/src/components/editor/theme.ts(0 hunks)frontend/packages/console-shared/src/components/links/ExternalLinkButton.tsx(1 hunks)frontend/packages/console-shared/src/utils/multiselectdropdown.tsx(1 hunks)frontend/packages/console-telemetry-plugin/src/components/TelemetryConfiguration.tsx(0 hunks)frontend/packages/console-telemetry-plugin/src/components/TelemetryUserPreferenceDropdown.tsx(0 hunks)frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx(0 hunks)frontend/public/components/masthead/masthead.tsx(1 hunks)frontend/public/components/modals/edit-yaml-settings-modal.tsx(5 hunks)frontend/public/components/utils/horizontal-nav.tsx(1 hunks)frontend/public/components/utils/resource-log.tsx(1 hunks)frontend/public/style/_overrides.scss(1 hunks)frontend/public/style/_vars.scss(1 hunks)frontend/scripts/check-patternfly-modules.sh(1 hunks)
💤 Files with no reviewable changes (5)
- frontend/packages/console-telemetry-plugin/src/components/TelemetryUserPreferenceDropdown.tsx
- frontend/packages/console-telemetry-plugin/src/components/TelemetryConfiguration.tsx
- frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx
- frontend/packages/console-shared/src/components/editor/theme.ts
- frontend/packages/console-app/src/components/access-modes/access-mode.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/public/style/_vars.scssfrontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsxfrontend/package.jsonfrontend/public/components/masthead/masthead.tsxfrontend/packages/console-shared/src/utils/multiselectdropdown.tsxfrontend/scripts/check-patternfly-modules.shfrontend/public/components/modals/edit-yaml-settings-modal.tsxfrontend/packages/console-shared/src/components/links/ExternalLinkButton.tsxfrontend/public/components/utils/resource-log.tsxfrontend/packages/console-app/src/components/favorite/FavoriteButton.tsxfrontend/packages/console-app/src/components/favorite/FavoriteNavItems.tsxfrontend/public/components/utils/horizontal-nav.tsxfrontend/packages/console-dynamic-plugin-sdk/README.mdfrontend/public/style/_overrides.scss
🔇 Additional comments (14)
frontend/public/components/utils/resource-log.tsx (1)
356-356: Settings icon rendering is supported in PatternFly 6.4.0 — change is safe.The refactor to use the
isSettingsprop aligns with PatternFly 6 patterns. Verification confirms that theisSettingsprop on MenuToggle is a supported feature in PF 6.4.0 with no known rendering issues. The change safely replaces custom icon rendering and simplifies the code.frontend/packages/console-dynamic-plugin-sdk/README.md (1)
219-222: LGTM!The documentation correctly informs plugin developers about the PatternFly 6.4 upgrade.
frontend/public/components/utils/horizontal-nav.tsx (1)
196-196: Verify visual consistency after removing custom CSS class.The change replaces
className="co-horizontal-nav"withusePageInsetsprop, aligning with PatternFly 6 patterns. This corresponds to the CSS removal documented in the SDK README. Verify that the tabs render with correct spacing and alignment, especially checking the inset behavior matches the previous custom styling.frontend/public/components/masthead/masthead.tsx (1)
61-65: LGTM!The refactor to use
PageToggleButtonwithisHamburgerButtonprop is cleaner and aligns with PatternFly 6 component APIs, replacing the previous manual icon rendering.frontend/packages/console-shared/src/utils/multiselectdropdown.tsx (1)
20-20: LGTM!The type change to
React.UIEventcorrectly aligns with PatternFly'sonSelectionChangehandler signature and removes the need for type suppression.frontend/public/style/_overrides.scss (1)
96-103: Verify the button ripple removal and custom active state.This override removes PatternFly's default button ripple animation and adds a custom active state background. Confirm that:
- Removing the ripple animation is an intentional UX decision
- The custom active state doesn't conflict with PatternFly 6.4's button variants
- Accessibility considerations (focus indicators, etc.) remain intact
frontend/public/style/_vars.scss (1)
25-25: LGTM!The change to
BorderInlineStartWidthuses the more precise logical property token, improving RTL language support and aligning with PatternFly 6 token conventions.frontend/scripts/check-patternfly-modules.sh (1)
56-56: Changes verified and correctly aligned with dependencies.The verification confirms that the script changes are correct:
@patternfly/react-drag-drop(version^6.4.0) is present infrontend/package.jsonat line 145, and@patternfly/react-consolehas been removed as expected. The script's check for@patternfly/react-drag-drop:6aligns with the actual dependency updates.frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx (1)
41-41: API usage verified as correct for PatternFly CodeEditor v6.4.0.The
isDarkThemeprop is a valid boolean API for PatternFly CodeEditor v6, with the description: "Flag indicating the editor is styled using Monaco's dark theme." The implementation correctly maps the theme context to this prop with the conditionaltheme === 'dark', ensuring the editor receives the appropriate boolean value. The code change is sound and follows the proper PatternFly v6 pattern. Runtime theme rendering will be verified through testing and QA processes.frontend/packages/console-app/src/components/favorite/FavoriteNavItems.tsx (1)
80-81: Verify favorites button behavior consistency.Similar to FavoriteButton.tsx, this component now uses
isFavoriteandisFavoritedprops. Ensure the favorites variant buttons work correctly across both components, given the known issues flagged in the PR comments.frontend/package.json (1)
137-154: PatternFly ecosystem upgrade looks well-structured.The upgrade to PatternFly v6.4.0 across the ecosystem is comprehensive and uses appropriate version ranges. The addition of new packages (@patternfly/patternfly, @patternfly/quickstarts, @patternfly/react-catalog-view-extension, @patternfly/react-drag-drop) aligns with the PF6 architecture.
frontend/public/components/modals/edit-yaml-settings-modal.tsx (2)
14-14: Good migration to PatternFly icons.The transition from Font Awesome icons to PatternFly icons (ICursorIcon, MouseIcon) and making the Icon prop required improves consistency with the PF6 design system.
Also applies to: 29-29, 37-37, 86-86, 105-105
143-144: Verify CodeEditorControlisSettingsbehavior in your PatternFly version.GitHub issue #12065 (CodeEditorControl must supply
iconeven when usingisSettings) is marked Done under the 2025.Q4 milestone, but the PatternFly 6.4 release notes do not list this issue or a corresponding fix explicitly. The code pattern at lines 143–144 matches the reported issue description.Before merging, confirm:
- Your project's actual
@patternfly/react-code-editorversion- Whether issue #12065 is resolved in that version
- Test the
isSettingsvariant behavior in your environmentfrontend/packages/console-app/src/components/favorite/FavoriteButton.tsx (1)
140-141: Let me search for more specific information about PatternFly 6.4.0 release and its fixes.Let me try a direct search for the specific GitHub issue:
Unable to verify the referenced GitHub issue—please confirm the issue number and repository.
The web search could not locate the specific issue about "favorites variant buttons" referenced in your comment. Issue #7894 in patternfly-react is about deprecated babel-eslint, not favorites buttons, suggesting the link may be incorrect or point to the wrong repository.
While PatternFly 6.4.0 has been released and includes a favorite button animation feature where the star "pulses" and fills with color, I cannot verify whether the specific favorites button issues you're concerned about have been resolved in this version.
Please confirm the correct GitHub issue number and repository so the verification can be completed.
frontend/packages/console-shared/src/components/links/ExternalLinkButton.tsx
Show resolved
Hide resolved
logonoff
force-pushed
the
OCPBUGS-62638
branch
from
8f16bd2 to
f01b6ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/public/components/modals/edit-yaml-settings-modal.tsx (1)
142-148: Code editor settings control change looks good; consider omittingicon={undefined}Using
CodeEditorControlwithisSettingsmatches the intent of this control and removes the legacy Cog icon. If PF’s API treatsiconas optional whenisSettingsis true, you can likely dropicon={undefined}for slightly clearer code, but behavior should already be correct assuming PF handlesundefinedsafely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
frontend/package.json(1 hunks)frontend/packages/console-app/src/components/access-modes/access-mode.tsx(0 hunks)frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx(1 hunks)frontend/packages/console-app/src/components/favorite/FavoriteNavItems.tsx(1 hunks)frontend/packages/console-dynamic-plugin-sdk/README.md(1 hunks)frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx(1 hunks)frontend/packages/console-shared/src/components/editor/theme.ts(0 hunks)frontend/packages/console-shared/src/components/links/ExternalLinkButton.tsx(2 hunks)frontend/packages/console-shared/src/utils/multiselectdropdown.tsx(1 hunks)frontend/packages/console-telemetry-plugin/src/components/TelemetryConfiguration.tsx(0 hunks)frontend/packages/console-telemetry-plugin/src/components/TelemetryUserPreferenceDropdown.tsx(0 hunks)frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx(0 hunks)frontend/public/components/masthead/masthead.tsx(1 hunks)frontend/public/components/modals/edit-yaml-settings-modal.tsx(5 hunks)frontend/public/components/utils/horizontal-nav.tsx(1 hunks)frontend/public/components/utils/resource-log.tsx(1 hunks)frontend/public/style/_overrides.scss(1 hunks)frontend/public/style/_vars.scss(1 hunks)frontend/scripts/check-patternfly-modules.sh(1 hunks)
💤 Files with no reviewable changes (5)
- frontend/packages/console-app/src/components/access-modes/access-mode.tsx
- frontend/packages/console-telemetry-plugin/src/components/TelemetryUserPreferenceDropdown.tsx
- frontend/packages/console-shared/src/components/editor/theme.ts
- frontend/packages/console-telemetry-plugin/src/components/TelemetryConfiguration.tsx
- frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/packages/console-shared/src/utils/multiselectdropdown.tsx
- frontend/public/components/utils/horizontal-nav.tsx
- frontend/packages/console-dynamic-plugin-sdk/README.md
- frontend/scripts/check-patternfly-modules.sh
- frontend/public/style/_overrides.scss
- frontend/public/components/masthead/masthead.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/console-app/src/components/favorite/FavoriteNavItems.tsxfrontend/package.jsonfrontend/public/components/modals/edit-yaml-settings-modal.tsxfrontend/packages/console-shared/src/components/links/ExternalLinkButton.tsxfrontend/public/style/_vars.scssfrontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsxfrontend/public/components/utils/resource-log.tsxfrontend/packages/console-app/src/components/favorite/FavoriteButton.tsx
🔇 Additional comments (9)
frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx (1)
41-41: Verification confirms implementation is correct. No issues found.The ThemeContext is properly typed to provide only
'dark'or'light'string values. The comparisontheme === 'dark'at line 41 correctly handles all cases:
- If theme is
'dark'→isDarkThemeistrue- If theme is
'light'ornull/undefined(initial load) →isDarkThemeisfalseThe migration to PatternFly's
isDarkThemeboolean prop is implemented correctly.frontend/public/components/utils/resource-log.tsx (1)
349-361: Menu toggleisSettingsusage looks correctUsing
isSettingson the optionsMenuTogglealigns with PF6 patterns and keeps behavior unchanged (state and handlers are intact). No issues from a logic or accessibility standpoint given the existingaria-labeland visible text.frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx (1)
136-149: Favorite button state wiring is consistent
isFavoriteandisFavorited={isStarred}correctly mirror the existing favorite state and accessibility (aria-pressed, tooltip). Behavior and telemetry remain unchanged.frontend/public/style/_vars.scss (1)
23-27: Directional gutter token change is reasonableSwitching to
--pf-v6-c-page__main-container--BorderInlineStartWidthfor the horizontal gutter appears consistent with PF’s directional border tokens and should avoid over/under-compensating when only the inline-start border is present.frontend/packages/console-app/src/components/favorite/FavoriteNavItems.tsx (1)
75-90: Unfavorite button correctly marked as favoritedMarking the nav unfavorite control with
isFavoriteandisFavoritedmatches its visual state (these are existing favorites) and doesn’t alter behavior; the click handler still only removes the item.frontend/package.json (1)
136-155: PatternFly dependency alignment looks consistent; confirm log‑viewer versionThe PF 6.4.x dependency set here is coherent and matches the rest of the PR’s component updates. The one outlier is
@patternfly/react-log-viewer@^6.3.0; that may be intentional (e.g., 6.4.0 not yet available or incompatible), but it’s worth double‑checking against PF’s release notes to avoid subtle API drift.frontend/packages/console-shared/src/components/links/ExternalLinkButton.tsx (1)
1-17: ExternalLinkButton wrapper now preserves callericonPropsSwitching to
PfExternalLinkButtonand mergingiconPropsasiconProps={{ title: t('(Opens in new tab)'), ...iconProps }}fixes the previous issue where caller icon props (e.g.,className) were discarded. Callers can still override the title if necessary, and the type re‑export keeps the public surface aligned with the PF component.frontend/public/components/modals/edit-yaml-settings-modal.tsx (2)
24-30: MakingIconrequired improves ConfigModalItem clarityGiven all usages supply an icon and
ConfigModalItemis local to this file, requiringIconinConfigModalItemPropstightens the contract without introducing breakage.
72-88: Tooltip and sticky‑scroll config items correctly provide explicit iconsPassing
ICursorIconandMouseIconintoConfigModalItemmatches the new requiredIconprop and keeps the UI semantics clear (cursor for tooltips, mouse for sticky scroll).
logonoff
force-pushed
the
OCPBUGS-62638
branch
from
f01b6ef to
95a1776
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/scripts/check-patternfly-modules.sh (1)
47-65: Consider also including new PF 6 packages inPKGS_TO_CHECKAdding
@patternfly/react-drag-drop:6here is good. Sincepackage.jsonnow also depends on@patternfly/react-data-viewand@patternfly/react-templatesat 6.x, you may want to include them inPKGS_TO_CHECKso the “single major version” guard applies consistently to all first-class PF 6 packages.Example:
PKGS_TO_CHECK=( '@patternfly/patternfly:6' '@patternfly/quickstarts:6' '@patternfly/react-catalog-view-extension:6' '@patternfly/react-charts:8' '@patternfly/react-code-editor:6' '@patternfly/react-component-groups:6' '@patternfly/react-core:6' '@patternfly/react-drag-drop:6' + '@patternfly/react-data-view:6' '@patternfly/react-icons:6' '@patternfly/react-log-viewer:6' '@patternfly/react-styles:6' '@patternfly/react-table:6' + '@patternfly/react-templates:6' '@patternfly/react-tokens:6' '@patternfly/react-topology:6' '@patternfly/react-user-feedback:6' '@patternfly/react-virtualized-extension:6' )frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx (1)
22-55: Ensure ThemeContext remains authoritative forisDarkThemeRight now
isDarkThemeis set before{...props}, so a caller passingisDarkThemeinpropswould override the ThemeContext-based value. If the intent is for the console theme to drive the editor theme, consider letting the context value win:- <CodeEditor - copyButtonAriaLabel={t('Copy code to clipboard')} + <CodeEditor + copyButtonAriaLabel={t('Copy code to clipboard')} copyButtonSuccessTooltipText={t('Content copied to clipboard')} copyButtonToolTipText={t('Copy code to clipboard')} downloadButtonAriaLabel={t('Download code')} downloadButtonToolTipText={t('Download code')} shortcutsPopoverButtonText={t('Shortcuts')} uploadButtonAriaLabel={t('Upload code')} uploadButtonToolTipText={t('Upload code')} emptyStateBody={t('Drag and drop a file or upload one.')} emptyStateButton={t('Browse')} emptyStateLink={t('Start from scratch')} emptyStateTitle={t('Start editing')} - isDarkTheme={theme === 'dark'} - {...props} + {...props} + isDarkTheme={theme === 'dark'} className={css('co-code-editor', props.className)}This keeps the component’s documented behavior (“theme applied” from the console theme) consistent even if the prop surface grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
frontend/package.json(1 hunks)frontend/packages/console-app/src/components/access-modes/access-mode.tsx(0 hunks)frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx(1 hunks)frontend/packages/console-shared/src/components/editor/theme.ts(0 hunks)frontend/packages/console-shared/src/components/links/ExternalLinkButton.tsx(2 hunks)frontend/packages/console-shared/src/utils/multiselectdropdown.tsx(1 hunks)frontend/packages/console-telemetry-plugin/src/components/TelemetryConfiguration.tsx(0 hunks)frontend/packages/console-telemetry-plugin/src/components/TelemetryUserPreferenceDropdown.tsx(0 hunks)frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx(0 hunks)frontend/public/style/_overrides.scss(1 hunks)frontend/public/style/_vars.scss(1 hunks)frontend/scripts/check-patternfly-modules.sh(1 hunks)
💤 Files with no reviewable changes (5)
- frontend/packages/console-telemetry-plugin/src/components/TelemetryUserPreferenceDropdown.tsx
- frontend/packages/console-app/src/components/access-modes/access-mode.tsx
- frontend/packages/console-shared/src/components/editor/theme.ts
- frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx
- frontend/packages/console-telemetry-plugin/src/components/TelemetryConfiguration.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/packages/console-shared/src/components/links/ExternalLinkButton.tsx
- frontend/public/style/_overrides.scss
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/public/style/_vars.scssfrontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsxfrontend/packages/console-shared/src/utils/multiselectdropdown.tsxfrontend/scripts/check-patternfly-modules.shfrontend/package.json
🔇 Additional comments (3)
frontend/packages/console-shared/src/utils/multiselectdropdown.tsx (1)
20-20: LGTM: Event type broadened to align with PatternFly v6.4.0.The change from
React.MouseEvent | React.ChangeEventtoReact.UIEventcorrectly aligns with the PatternFly v6.4.0 upgrade. The broader base type safely encompasses all UI event types while maintaining full compatibility with the existingpreventDefault()call.frontend/public/style/_vars.scss (1)
25-25: Token verified—no issues found.The PatternFly v6 token
--pf-v6-c-page__main-container--BorderInlineStartWidthexists with a default value of 4px, confirming the change fromBorderWidthis correct and properly supports PatternFly 6.4.0's logical properties model for bidirectional layout support.frontend/package.json (1)
136-155: PatternFly dependency alignment looks consistent; rely on CI/QA to cover integrationThe PatternFly dependencies here are coherently aligned on the 6.x (and 8.x for charts) lines and match the intent of the PF 6.4.0 upgrade. From a package graph perspective this looks good and works with the
check-patternfly-modules.shsingle-major guard you have in place.Please ensure CI (including dynamic plugin tests and create-form flows) runs clean against this dependency set, since any subtle React/PF peerDependency or styling regressions will surface there rather than in static review.
|
|
||
| // PatternFly overrides | ||
|
|
||
| .pf-v6-c-button { |
There was a problem hiding this comment.
Let's discuss this as a team. I don't love this animation, either, but we need to have agreement on deviating from PatternFly.
logonoff
force-pushed
the
OCPBUGS-62638
branch
from
95a1776 to
9caaec3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/public/style/_overrides.scss (1)
96-103: Global.pf-v6-c-buttonbackground override may impact all variants/statesThis removes the ripple but also overrides the background for every PF v6 button variant and its active state. That’s a pretty broad hook and may subtly change plain/link/tertiary buttons or future PF tweaks (hover/disabled/focus visuals).
If possible, consider scoping this to only the variants you want to change (e.g. excluding
.pf-m-plain/.pf-m-link) or at least verify the main variants across themes and states to ensure there are no regressions.-.pf-v6-c-button { +.pf-v6-c-button:not(.pf-m-plain):not(.pf-m-link) { // Remove button ripple animation background: var(--pf-v6-c-button--BackgroundColor); &:active { background: linear-gradient( color-mix(in srgb, currentcolor 15%, transparent) ), var(--pf-v6-c-button--BackgroundColor); } }frontend/package.json (1)
136-154: PF 6.4 dependency alignment looks coherent; prioritize regression + plugin testingThe PatternFly dependency block is now consistently on 6.x (mostly 6.4.0), which matches the upgrade intent and should keep tokens/components in sync across the app.
Given how cross‑cutting this change is, it’d be good to:
- Re‑run
check-patternfly-modules.shand ensure no stray PF versions slip in via transitive deps.- Do targeted regression passes on create forms, topology, quick starts, and console/dynamic plugins as mentioned in the PR comments, since subtle PF behavior/spacing changes often only show up in those flows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
frontend/package.json(1 hunks)frontend/packages/console-app/src/components/access-modes/access-mode.tsx(0 hunks)frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx(1 hunks)frontend/packages/console-app/src/components/favorite/FavoriteNavItems.tsx(1 hunks)frontend/packages/console-dynamic-plugin-sdk/README.md(1 hunks)frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx(1 hunks)frontend/packages/console-shared/src/components/editor/theme.ts(0 hunks)frontend/packages/console-shared/src/components/links/ExternalLinkButton.tsx(2 hunks)frontend/packages/console-shared/src/utils/multiselectdropdown.tsx(1 hunks)frontend/packages/console-telemetry-plugin/src/components/TelemetryConfiguration.tsx(0 hunks)frontend/packages/console-telemetry-plugin/src/components/TelemetryUserPreferenceDropdown.tsx(0 hunks)frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx(0 hunks)frontend/public/components/masthead/masthead.tsx(1 hunks)frontend/public/components/modals/edit-yaml-settings-modal.tsx(1 hunks)frontend/public/components/utils/horizontal-nav.tsx(1 hunks)frontend/public/components/utils/resource-log.tsx(1 hunks)frontend/public/style/_overrides.scss(1 hunks)frontend/public/style/_vars.scss(1 hunks)frontend/scripts/check-patternfly-modules.sh(1 hunks)
💤 Files with no reviewable changes (5)
- frontend/packages/console-shared/src/components/editor/theme.ts
- frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx
- frontend/packages/console-telemetry-plugin/src/components/TelemetryUserPreferenceDropdown.tsx
- frontend/packages/console-telemetry-plugin/src/components/TelemetryConfiguration.tsx
- frontend/packages/console-app/src/components/access-modes/access-mode.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/public/components/utils/horizontal-nav.tsx
- frontend/scripts/check-patternfly-modules.sh
- frontend/public/components/utils/resource-log.tsx
- frontend/packages/console-app/src/components/favorite/FavoriteNavItems.tsx
- frontend/packages/console-dynamic-plugin-sdk/README.md
- frontend/packages/console-shared/src/utils/multiselectdropdown.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/public/style/_vars.scssfrontend/package.jsonfrontend/public/components/modals/edit-yaml-settings-modal.tsxfrontend/public/style/_overrides.scssfrontend/packages/console-app/src/components/favorite/FavoriteButton.tsxfrontend/public/components/masthead/masthead.tsxfrontend/packages/console-shared/src/components/links/ExternalLinkButton.tsxfrontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx
🧬 Code graph analysis (1)
frontend/packages/console-shared/src/components/links/ExternalLinkButton.tsx (1)
frontend/__mocks__/i18next.ts (1)
t(8-14)
🔇 Additional comments (5)
frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx (1)
140-141: Verify visual regressions and dynamic plugin compatibility; PatternFly issue #7894 cannot be confirmed.The code correctly uses valid PatternFly v6.4.0 favorite button APIs. However:
- PatternFly issue #7894 referenced in PR comments could not be verified in the repositories or release notes
- Visual regression testing must be performed manually (as requested in PR discussion)
- Dynamic plugin compatibility must be verified in your test environment
The migration itself is technically sound and uses documented PatternFly v6.4.0 features.
frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx (1)
41-41: Verification confirms the migration is correct and complete.PatternFly v6.4.0 CodeEditor supports the
isDarkThemeprop, and the removal of customdefineThemesis confirmed. TheeditorPropsno longer contains a theme property, and the theme is now correctly derived fromThemeContextwith theisDarkTheme={theme === 'dark'}comparison on line 41.frontend/public/style/_vars.scss (1)
23-26: PF 6.4 page gutter token alignment looks good; confirm token name existsSwitching to
--pf-v6-c-page__main-container--BorderInlineStartWidthkeeps the gutter calculation tied to the new border token and should maintain layout consistency. Just double‑check the token name against PF 6.4 so we don’t silently drop this calc if the variable is missing.frontend/public/components/masthead/masthead.tsx (1)
61-65: HamburgerPageToggleButtonusage preserves behavior; validate accessibilitySwapping the custom
BarsIconchild forisHamburgerButtonwhile keepingonSidebarToggle/isSidebarOpenlooks consistent with the PF 6 API and should simplify future maintenance.Please sanity‑check that, with PF 6.4, this still exposes an appropriate accessible label and behaves correctly with keyboard and responsive nav (especially stacked vs inline masthead layouts).
frontend/public/components/modals/edit-yaml-settings-modal.tsx (1)
14-15: Switch to PatternFly icon components improves consistency; confirm icon availabilityMoving these controls to use
PaintRollerIcon,FontIcon,ICursorIcon, andMouseIconfrom@patternfly/react-icons(instead of localcreateIconwiring) is a good cleanup and keeps the YAML settings modal aligned with the PF 6.4 icon set.Please confirm at build/runtime that all four icons exist in the PF 6.4 package and render as expected in both light/dark themes so we don’t end up with missing icons or unexpected fallbacks.
Also applies to: 165-185, 197-221, 224-261
| icon={<ExternalLinkAltIcon title={t('(Opens in new tab)')} {...iconProps} />} | ||
| {...props} | ||
| /> | ||
| <PfExternalLinkButton iconProps={{ title: t('(Opens in new tab)'), ...iconProps }} {...props} /> |
There was a problem hiding this comment.
Fix iconProps spread order to prevent title override.
The spread order is reversed from the recommended fix in the past review. The current implementation sets title first, then spreads iconProps, which allows callers to override the translated title. For example, CloudShellDrawer.tsx passes iconProps={{ title: undefined }}, which would overwrite the accessibility title with undefined.
The component's purpose is to enforce a consistent, accessible title. Spread iconProps first, then set title last to ensure the translated title is always present.
Apply this diff:
- <PfExternalLinkButton iconProps={{ title: t('(Opens in new tab)'), ...iconProps }} {...props} />
+ <PfExternalLinkButton iconProps={{ ...iconProps, title: t('(Opens in new tab)') }} {...props} />📝 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.
| <PfExternalLinkButton iconProps={{ title: t('(Opens in new tab)'), ...iconProps }} {...props} /> | |
| <PfExternalLinkButton iconProps={{ ...iconProps, title: t('(Opens in new tab)') }} {...props} /> |
🤖 Prompt for AI Agents
In frontend/packages/console-shared/src/components/links/ExternalLinkButton.tsx
around line 16, the title for iconProps is set before spreading the incoming
iconProps which allows callers to overwrite or remove the accessibility title;
change the props order so you spread the incoming iconProps first and then set
title last (iconProps={{ ...iconProps, title: t('(Opens in new tab)') }}) to
ensure the translated title cannot be overridden while preserving other passed
iconProps.
|
/retest-required |
logonoff
force-pushed
the
OCPBUGS-62638
branch
2 times, most recently
from
d92e38a to
d63b11e
Compare
Co-Authored-By: logonoff <18614559+logonoff@users.noreply.github.com> Co-Authored-By: Robb Hamilton <895728+rhamilto@users.noreply.github.com>
logonoff
force-pushed
the
OCPBUGS-62638
branch
from
d63b11e to
0b35424
Compare
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@rhamilto: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
…rride can be removed
After