ECOPROJECT-4355 | refactor: changing code to adapt styles and colors to dark mode#572
Merged
openshift-merge-bot[bot] merged 1 commit intokubev2v:masterfrom Apr 16, 2026
Conversation
…to Dark mode Signed-off-by: Montse Ortega <mortegag@redhat.com>
📝 WalkthroughWalkthroughThis PR replaces hardcoded colors and theme values with PatternFly CSS variables across multiple UI components, enabling dynamic theme support and consistency. Changes affect assessment views, environmental views, reports, charts, and core components without modifying component logic or functionality. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/assessment/views/MigrationAssessmentStepsModal.tsx (1)
26-73: 🛠️ Refactor suggestion | 🟠 MajorRefactor inline styles to use
@emotion/css.The tab button styles use inline
styleprops, which violates the coding guideline. As per coding guidelines, all component styles must use@emotion/css(CSS-in-JS) rather than inline styles or plain CSS files.♻️ Proposed refactor using `@emotion/css`
Add the import and define styles at the top of the file:
+import { css } from "@emotion/css"; import { Modal, ModalBody,Define the button styles:
const tabButtonBase = css({ flex: 1, cursor: "pointer", borderRadius: "12px", padding: "12px 24px", fontSize: "14px", fontWeight: 500, color: "var(--pf-t--global--text--color--regular)", transition: "all 0.2s ease", }); const tabButtonActive = css({ border: "2px solid var(--pf-t--global--border--color--status--info--default)", backgroundColor: "var(--pf-t--global--background--color--status--info--default)", }); const tabButtonInactive = css({ border: "1px solid var(--pf-t--global--border--color--default)", backgroundColor: "var(--pf-t--global--background--color--primary--default)", });Then apply to buttons:
<div className="pf-v6-u-mb-lg" style={{ display: "flex", gap: "16px" }}> <button onClick={() => setActiveTab("existing")} - style={{ - flex: 1, - cursor: "pointer", - border: - activeTab === "existing" - ? "2px solid var(--pf-t--global--border--color--status--info--default)" - : "1px solid var(--pf-t--global--border--color--default)", - backgroundColor: - activeTab === "existing" - ? "var(--pf-t--global--background--color--status--info--default)" - : "var(--pf-t--global--background--color--primary--default)", - borderRadius: "12px", - padding: "12px 24px", - fontSize: "14px", - fontWeight: 500, - color: "var(--pf-t--global--text--color--regular)", - transition: "all 0.2s ease", - }} + className={`${tabButtonBase} ${activeTab === "existing" ? tabButtonActive : tabButtonInactive}`} type="button" > Existing environment </button> <button onClick={() => setActiveTab("new")} - style={{ - flex: 1, - cursor: "pointer", - border: - activeTab === "new" - ? "2px solid var(--pf-t--global--border--color--status--info--default)" - : "1px solid var(--pf-t--global--border--color--default)", - backgroundColor: - activeTab === "new" - ? "var(--pf-t--global--background--color--status--info--default)" - : "var(--pf-t--global--background--color--primary--default)", - borderRadius: "12px", - padding: "12px 24px", - fontSize: "14px", - fontWeight: 500, - color: "var(--pf-t--global--text--color--regular)", - transition: "all 0.2s ease", - }} + className={`${tabButtonBase} ${activeTab === "new" ? tabButtonActive : tabButtonInactive}`} type="button" > New environment </button>As per coding guidelines: "Use
@emotion/css(CSS-in-JS) for all component styles — do not create plain .css files".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/assessment/views/MigrationAssessmentStepsModal.tsx` around lines 26 - 73, The two tab buttons in MigrationAssessmentStepsModal (the elements using onClick={() => setActiveTab("existing")} and onClick={() => setActiveTab("new")}) currently use inline style props; refactor by importing css from "@emotion/css", create style classes (e.g., tabButtonBase, tabButtonActive, tabButtonInactive) that capture the shared and conditional styles shown in the diff (flex, cursor, borderRadius, padding, fontSize, fontWeight, color, transition, plus active vs inactive border/background), and replace the inline style prop with className that composes tabButtonBase plus tabButtonActive or tabButtonInactive depending on activeTab. Ensure the activeTab comparison logic (activeTab === "existing" / "new") is preserved when selecting the class.
🧹 Nitpick comments (2)
src/ui/assessment/views/AssessmentDetails.tsx (1)
90-100: Move these updated inline styles into Emotion classes for guideline compliance.The token values are correct, but these changed blocks still rely on inline
styleprops.♻️ Suggested refactor
+import { css } from "@emotion/css"; + +const sectionCard = css` + background: var(--pf-t--global--background--color--primary--default); + padding: 16px; + border-radius: 4px; +`; + +const sectionHeader = css` + border-bottom: 1px solid var(--pf-t--global--border--color--default); + padding-bottom: 8px; + margin-bottom: 16px; +`; + +const reportIcon = css` + color: var(--pf-t--global--text--color--link--default); +`; ... -<div style={{ background: "var(--pf-t--global--background--color--primary--default)", padding: "16px", borderRadius: "4px" }}> +<div className={sectionCard}> ... -<div style={{ borderBottom: "1px solid var(--pf-t--global--border--color--default)", paddingBottom: "8px", marginBottom: "16px" }}> +<div className={sectionHeader}> ... -<MonitoringIcon style={{ color: "var(--pf-t--global--text--color--link--default)" }} /> +<MonitoringIcon className={reportIcon} />As per coding guidelines
**/*.{ts,tsx}: Use@emotion/css(CSS-in-JS) for all component styles — do not create plain .css files.Also applies to: 179-190, 242-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/assessment/views/AssessmentDetails.tsx` around lines 90 - 100, AssessmentDetails contains inline style props (two blocks shown and additional instances at the regions you noted) that must be migrated to `@emotion/css` classes; create Emotion style definitions (e.g., const container = css({...}) and const headerBorder = css({...})) using the same token values ("var(--pf-t--global--background--color--primary--default)", padding "16px", borderRadius "4px", and the borderBottom token) and replace the inline style={...} usages in the AssessmentDetails component with className={container} and className={headerBorder}; repeat the same pattern for the other inline style occurrences you called out (around 179-190 and 242-247) so all styles use Emotion CSS-in-JS instead of inline style props.src/ui/report/views/ReportTable.tsx (1)
30-33: Extract the new table border/caption styles to Emotion classes.The token migration is right, but the changed styling is still inline and should be class-based.
♻️ Suggested refactor
+import { css, cx } from "@emotion/css"; + +const bordered = css` + border: 1px solid var(--pf-t--global--border--color--default); +`; + +const captionStyle = css` + font-weight: bold; + font-size: 14px; + text-align: left; + padding: 8px 16px; + color: var(--pf-t--global--text--color--regular); +`; ... -<Table ... style={{ border: noBorder ? "none" : "1px solid var(--pf-t--global--border--color--default)", borderRight: "none", ...style }}> +<Table + ... + className={cx(!noBorder && bordered)} + style={{ borderRight: "none", ...style }} +> ... -<caption style={{ ... , color: "var(--pf-t--global--text--color--regular)" }}> +<caption className={captionStyle}> ... -<Tr style={{ border: noBorder ? "none" : "1px solid var(--pf-t--global--border--color--default)" }}> +<Tr className={cx(!noBorder && bordered)}>As per coding guidelines
**/*.{ts,tsx}: Use@emotion/css(CSS-in-JS) for all component styles — do not create plain .css files.Also applies to: 44-44, 51-57, 65-67, 81-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/report/views/ReportTable.tsx` around lines 30 - 33, The ReportTable component currently uses inline style objects for the table border, caption and related cell styles; extract these into `@emotion/css` classes (e.g., tableBorder, tableNoBorder, captionStyle, cellNoRightBorder) and replace the inline style props with className assignments that switch based on the noBorder prop and other flags. Locate the inline styles in the ReportTable React function (the table element, caption element and cell elements that set border / borderRight / caption-related properties) and implement Emotion classes that use the token-based values (var(--pf-...)) and conditional application (apply tableNoBorder when noBorder is true, otherwise tableBorder), then import/use css from `@emotion/css` and remove the corresponding inline style objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/ui/assessment/views/MigrationAssessmentStepsModal.tsx`:
- Around line 26-73: The two tab buttons in MigrationAssessmentStepsModal (the
elements using onClick={() => setActiveTab("existing")} and onClick={() =>
setActiveTab("new")}) currently use inline style props; refactor by importing
css from "@emotion/css", create style classes (e.g., tabButtonBase,
tabButtonActive, tabButtonInactive) that capture the shared and conditional
styles shown in the diff (flex, cursor, borderRadius, padding, fontSize,
fontWeight, color, transition, plus active vs inactive border/background), and
replace the inline style prop with className that composes tabButtonBase plus
tabButtonActive or tabButtonInactive depending on activeTab. Ensure the
activeTab comparison logic (activeTab === "existing" / "new") is preserved when
selecting the class.
---
Nitpick comments:
In `@src/ui/assessment/views/AssessmentDetails.tsx`:
- Around line 90-100: AssessmentDetails contains inline style props (two blocks
shown and additional instances at the regions you noted) that must be migrated
to `@emotion/css` classes; create Emotion style definitions (e.g., const container
= css({...}) and const headerBorder = css({...})) using the same token values
("var(--pf-t--global--background--color--primary--default)", padding "16px",
borderRadius "4px", and the borderBottom token) and replace the inline
style={...} usages in the AssessmentDetails component with className={container}
and className={headerBorder}; repeat the same pattern for the other inline style
occurrences you called out (around 179-190 and 242-247) so all styles use
Emotion CSS-in-JS instead of inline style props.
In `@src/ui/report/views/ReportTable.tsx`:
- Around line 30-33: The ReportTable component currently uses inline style
objects for the table border, caption and related cell styles; extract these
into `@emotion/css` classes (e.g., tableBorder, tableNoBorder, captionStyle,
cellNoRightBorder) and replace the inline style props with className assignments
that switch based on the noBorder prop and other flags. Locate the inline styles
in the ReportTable React function (the table element, caption element and cell
elements that set border / borderRight / caption-related properties) and
implement Emotion classes that use the token-based values (var(--pf-...)) and
conditional application (apply tableNoBorder when noBorder is true, otherwise
tableBorder), then import/use css from `@emotion/css` and remove the corresponding
inline style objects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b5836db-9941-40ec-89b4-a31d2e799c80
📒 Files selected for processing (29)
src/ui/assessment/views/Assessment.tsxsrc/ui/assessment/views/AssessmentDetails.tsxsrc/ui/assessment/views/AssessmentsTable.tsxsrc/ui/assessment/views/MigrationAssessmentStepsModal.tsxsrc/ui/core/components/CustomEnterpriseIcon.tsxsrc/ui/core/components/FilterPill.tsxsrc/ui/core/components/MigrationChart.tsxsrc/ui/core/components/MigrationDonutChart.tsxsrc/ui/environment/views/AgentStatusView.tsxsrc/ui/environment/views/SourcesTable.tsxsrc/ui/environment/views/VersionStatus.tsxsrc/ui/home/views/HowDoesItWork.tsxsrc/ui/report/views/ExampleReport.tsxsrc/ui/report/views/ExportReportButton.tsxsrc/ui/report/views/ReportPieChart.tsxsrc/ui/report/views/ReportTable.tsxsrc/ui/report/views/assessment-report/ClustersOverview.tsxsrc/ui/report/views/assessment-report/CpuAndMemoryOverview.tsxsrc/ui/report/views/assessment-report/Datastores.tsxsrc/ui/report/views/assessment-report/ErrorTable.tsxsrc/ui/report/views/assessment-report/HostsOverview.tsxsrc/ui/report/views/assessment-report/NetworkOverview.tsxsrc/ui/report/views/assessment-report/OSDistribution.tsxsrc/ui/report/views/assessment-report/StorageOverview.tsxsrc/ui/report/views/assessment-report/VMMigrationStatus.tsxsrc/ui/report/views/assessment-report/WarningsTable.tsxsrc/ui/report/views/assessment-report/styles.tssrc/ui/report/views/cluster-sizer/ComplexityResult.tsxsrc/ui/report/views/cluster-sizer/RecommendationTemplate.tsx
Collaborator
|
Hey @ammont82, PatternFly’s key requirement here is indeed the use of semantic tokens, so dark mode is handled automatically, and that part is nicely done. One thing I wanted to raise for discussion is how we consume tokens in the codebase. PatternFly supports both React tokens and CSS variables, and both are valid approaches depending on the context. Right now we’re using a mix of the two patterns in similar places, which can make the code a bit inconsistent and harder to reason about over time. It might be worth aligning on a more consistent guideline, for example:
Not a blocker for this patch at all, just something we may want to standardize gradually. /lgtm |
Collaborator
Author
@rh-gvincent thanks for your comments. I think it's good idea to have a guideline for this. We can talk about that when you want. |
Collaborator
Author
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ammont82 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
dark_mode.mp4
Summary by CodeRabbit