MGMT-20616: Refactor Header component to use not deprecated components of patternfly#2950
Conversation
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
WalkthroughThe Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/assisted-ui/src/components/Header.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "@openshift-assisted/eslint-config" to extend from. Please check that the name of the config is correct. The config "@openshift-assisted/eslint-config" was referenced from the config file in "/apps/assisted-ui/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/assisted-ui/src/components/Header.tsx (1)
7-7: Avoid inline styles; use PatternFly utility classes.Relying on inline
stylemakes overrides harder to manage. You can leverage PF utility classes for consistent theming:- <Masthead style={{ display: 'flex', justifyContent: 'space-between' }}> + <Masthead className="pf-u-display-flex pf-u-justify-content-space-between">This also enables easier customization via your global CSS or theming tokens.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/assisted-ui/src/components/Header.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/assisted-ui/src/components/Header.tsx (2)
apps/assisted-ui/src/components/FeedbackButton.tsx (1)
FeedbackButton(6-14)apps/assisted-ui/src/components/AboutButton.tsx (1)
AboutButton(6-18)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: circular-deps
- GitHub Check: unit-tests
- GitHub Check: translation-files
- GitHub Check: tests
- GitHub Check: format
- GitHub Check: lint
🔇 Additional comments (3)
apps/assisted-ui/src/components/Header.tsx (3)
2-2: Removed deprecated imports; approved change.The switch from the deprecated
PageHeaderandPageHeaderToolsimports to the currentMastheadcomponents aligns well with PatternFly’s migration guide and improves maintainability.
8-16: Verify logo link behavior and asset resolution.
- The previous
logoProps={{ href: '/' }}was removed, so the brand logo is no longer clickable. Please confirm if it should still navigate to the home/dashboard route. If so, wrap<Brand>in a React Router<Link>or re-add anhrefon the containing element.- Referencing
"/logo.svg"directly assumes a root‐mounted public path. In non-root deployments, this can break. Consider importing the SVG and using it as a module:-import { Brand } from '@patternfly/react-core'; +import { Brand } from '@patternfly/react-core'; +import logo from 'assets/logo.svg'; … - <Brand src="/logo.svg" alt="…"> + <Brand src={logo} alt="Assisted Installer logo">
17-20: Confirm correct container for action buttons.In PatternFly’s masthead pattern, header control buttons (feedback, about) are commonly placed within
<MastheadTools>(or nested under<MastheadContent>). You’re using<MastheadMain>—please verify against the PF docs that this component is semantically appropriate. If not, replace with the recommended container:<MastheadContent> <MastheadTools> <FeedbackButton /> <AboutButton /> </MastheadTools> </MastheadContent>
|
@ElayAharoni: This pull request references MGMT-20616 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ElayAharoni, jgyselov 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 |
f8d67af
into
openshift-assisted:master
…fly (openshift-assisted#2950) Signed-off-by: Elay Aharoni <elayaha@gmail.com>
https://issues.redhat.com/browse/MGMT-20616
used Patternfly's Masthead components instead of PageHeader
Summary by CodeRabbit