feat(react-charts): add high contrast#12419
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR updates Victory and ECharts chart themes to use CSS variable tokens for stroke color and strokeWidth (and some fillOpacity) across area, bar, legend, pie, bullet, and donut components; switches skeleton color scales to chart_skeleton tokens; and bumps ChangesChart Theme Token Styling
Dependency Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
Preview: https://pf-react-pr-12419.surge.sh A11y report: https://pf-react-pr-12419-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/react-charts/src/victory/components/ChartTheme/themes/base-theme.ts (2)
175-178: 💤 Low value
area.style.data.strokeWidthnot migrated to.varWithin the same
datablock,fillOpacitywas migrated to.varfor high-contrast CSS-variable support, butstrokeWidthon line 178 still uses.value. Sincestrokeis intentionally commented out, this has no current visual impact, but it leaves the token unable to respond to CSS variable overrides ifstrokeis re-enabled later.🔧 Proposed fix
- strokeWidth: chart_area_stroke_Width.value + strokeWidth: chart_area_stroke_Width.var🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-charts/src/victory/components/ChartTheme/themes/base-theme.ts` around lines 175 - 178, The area style's data block uses chart_area_stroke_Width.value for strokeWidth while fillOpacity was migrated to chart_area_Opacity.var; update the area.style.data property named strokeWidth to use chart_area_stroke_Width.var instead of .value so the token responds to CSS-variable overrides (check the area.style.data object and replace the .value usage on strokeWidth with .var).
522-528: ⚡ Quick winDonut variants should use variant-specific stroke tokens to match the per-variant naming convention
Lines 525–526, 539–540, and 553–554 in
donutThresholdDynamic,donutThresholdStatic, anddonutUtilizationall reference the basechart_donut_pie_data_stroke_Colorandchart_donut_pie_data_stroke_Widthtokens. However, each variant already imports its own dimensional tokens (chart_donut_threshold_dynamic_pie_Height,chart_donut_threshold_static_pie_Width, etc.), establishing a per-variant naming convention. The stroke tokens should follow the same pattern to allow independent customization per variant—for example,chart_donut_threshold_dynamic_pie_data_stroke_ColorfordonutThresholdDynamic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-charts/src/victory/components/ChartTheme/themes/base-theme.ts` around lines 522 - 528, The donut variants (donutThresholdDynamic, donutThresholdStatic, donutUtilization) are incorrectly using the generic chart_donut_pie_data_stroke_Color and chart_donut_pie_data_stroke_Width tokens; update each variant to use its own per-variant stroke tokens (e.g., replace chart_donut_pie_data_stroke_Color and chart_donut_pie_data_stroke_Width in donutThresholdDynamic with chart_donut_threshold_dynamic_pie_data_stroke_Color and chart_donut_threshold_dynamic_pie_data_stroke_Width respectively) and make analogous replacements in donutThresholdStatic and donutUtilization to use chart_donut_threshold_static_pie_data_stroke_* and chart_donut_utilization_pie_data_stroke_* tokens so each variant can be customized independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/react-charts/src/victory/components/ChartTheme/themes/base-theme.ts`:
- Around line 60-61: The imports for six missing token modules
(chart_legend_data_stroke_Color, chart_legend_data_stroke_Width,
chart_donut_pie_data_stroke_Color, chart_donut_pie_data_stroke_Width,
chart_bullet_bar_stroke_Color, chart_bullet_bar_stroke_Width) are invalid and
will break the build; either add corresponding token files to
`@patternfly/react-tokens` exporting these symbols, or remove/replace their
imports in base-theme.ts: update the import list to stop importing those six
identifiers and adjust any usages in ChartTheme (e.g., where
chart_legend_data_stroke_Color / _Width, chart_donut_pie_data_stroke_*,
chart_bullet_bar_stroke_*) to use existing tokens or explicit values so no
unresolved module paths remain.
In `@packages/react-core/src/components/Hero/Hero.tsx`:
- Line 38: The Hero component removed the hasNoGlass prop from the
typed/destructured props but still spreads ...props onto the outer <div>,
causing hasNoGlass to leak to the DOM and breaking the public API; restore
support by re-introducing hasNoGlass into the component props (and its
TypeScript interface) and destructure it out of props (e.g. function Hero({
hasNoGlass, ...props }) or the existing props interface) so you can consume it
internally and not pass it through the ...props spread, or if you intend to
deprecate it, explicitly handle it and strip it before spreading while marking
it deprecated in the props type/comment; update references to hasNoGlass inside
Hero accordingly (and ensure tests/types reflect the restored prop).
---
Nitpick comments:
In
`@packages/react-charts/src/victory/components/ChartTheme/themes/base-theme.ts`:
- Around line 175-178: The area style's data block uses
chart_area_stroke_Width.value for strokeWidth while fillOpacity was migrated to
chart_area_Opacity.var; update the area.style.data property named strokeWidth to
use chart_area_stroke_Width.var instead of .value so the token responds to
CSS-variable overrides (check the area.style.data object and replace the .value
usage on strokeWidth with .var).
- Around line 522-528: The donut variants (donutThresholdDynamic,
donutThresholdStatic, donutUtilization) are incorrectly using the generic
chart_donut_pie_data_stroke_Color and chart_donut_pie_data_stroke_Width tokens;
update each variant to use its own per-variant stroke tokens (e.g., replace
chart_donut_pie_data_stroke_Color and chart_donut_pie_data_stroke_Width in
donutThresholdDynamic with chart_donut_threshold_dynamic_pie_data_stroke_Color
and chart_donut_threshold_dynamic_pie_data_stroke_Width respectively) and make
analogous replacements in donutThresholdStatic and donutUtilization to use
chart_donut_threshold_static_pie_data_stroke_* and
chart_donut_utilization_pie_data_stroke_* tokens so each variant can be
customized independently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d0d264f-f37a-47f3-b01f-f84bbf4bd76c
📒 Files selected for processing (2)
packages/react-charts/src/victory/components/ChartTheme/themes/base-theme.tspackages/react-core/src/components/Hero/Hero.tsx
dlabrecq
left a comment
dlabrecq
left a comment
There was a problem hiding this comment.
Looks ok, but concerned about CodeRabbit's base-theme comment regarding missing token module files. Guessing that's because react-tokens needs to be updated first? Whenever that's ready, I'd like to run the examples to verify the new BaseComponentTheme properties
| }, | ||
| scatter: { | ||
| style: { | ||
| data: { | ||
| stroke: chart_bullet_bar_stroke_Color.var, | ||
| strokeWidth: chart_bullet_bar_stroke_Width.var | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure about the scatter props. This is new here and need to test visually
There was a problem hiding this comment.
@dlabrecq as far as I can tell, the coderabbit comment about imports was just out of date. The build and everything was passing and all of the visual regression tests passed. I requested a re-review and it resoled that comment and didn't flag it again.
Not sure about the scatter props. This is new here and need to test visually
You can see that one here - https://pf-react-pr-12419.surge.sh/components/charts/bullet-chart#primary-measure-dot
|
@coderabbitai can you re-review this PR? |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-charts/src/victory/components/ChartTheme/themes/base-theme.ts (1)
504-557: ⚡ Quick winExtract duplicated donut stroke style into a shared constant.
Line 510–Line 515, Line 523–Line 528, Line 537–Line 542, and Line 551–Line 556 repeat the same
style.dataobject. Pulling this into a constant will reduce drift risk when token mappings change again.♻️ Suggested refactor
+const DONUT_PIE_DATA_STROKE_STYLE = { + data: { + stroke: chart_donut_pie_data_stroke_Color.var, + strokeWidth: chart_donut_pie_data_stroke_Width.var + } +}; donut: { pie: { height: chart_donut_pie_Height.value, padding: chart_donut_pie_Padding.value, padAngle: chart_donut_pie_angle_Padding.value, width: chart_donut_pie_Width.value, - style: { - data: { - stroke: chart_donut_pie_data_stroke_Color.var, - strokeWidth: chart_donut_pie_data_stroke_Width.var - } - } + style: DONUT_PIE_DATA_STROKE_STYLE } as any },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-charts/src/victory/components/ChartTheme/themes/base-theme.ts` around lines 504 - 557, The pie charts for donut, donutThresholdDynamic, donutThresholdStatic, and donutUtilization all repeat the same style.data object (using chart_donut_pie_data_stroke_Color.var and chart_donut_pie_data_stroke_Width.var); extract that object into a shared constant (e.g., const donutStrokeStyle = { data: { stroke: chart_donut_pie_data_stroke_Color.var, strokeWidth: chart_donut_pie_data_stroke_Width.var } }) and replace each pie.style with a reference to that constant (pie.style = donutStrokeStyle) so the four occurrences in the pie configs (inside donut, donutThresholdDynamic, donutThresholdStatic, donutUtilization) reuse the single constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/react-charts/src/victory/components/ChartTheme/themes/base-theme.ts`:
- Around line 504-557: The pie charts for donut, donutThresholdDynamic,
donutThresholdStatic, and donutUtilization all repeat the same style.data object
(using chart_donut_pie_data_stroke_Color.var and
chart_donut_pie_data_stroke_Width.var); extract that object into a shared
constant (e.g., const donutStrokeStyle = { data: { stroke:
chart_donut_pie_data_stroke_Color.var, strokeWidth:
chart_donut_pie_data_stroke_Width.var } }) and replace each pie.style with a
reference to that constant (pie.style = donutStrokeStyle) so the four
occurrences in the pie configs (inside donut, donutThresholdDynamic,
donutThresholdStatic, donutUtilization) reuse the single constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01a8dc0f-fc17-45b6-b921-6ad51c299a60
⛔ Files ignored due to path filters (29)
packages/react-charts/src/victory/components/Chart/__snapshots__/Chart.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartArea/__snapshots__/ChartArea.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartAxis/__snapshots__/ChartAxis.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartBar/__snapshots__/ChartBar.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartBoxPlot/__snapshots__/ChartBoxPlot.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartBullet/__snapshots__/ChartBullet.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartBullet/__snapshots__/ChartBulletGroupTitle.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartBullet/__snapshots__/ChartBulletPrimaryDotMeasure.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartBullet/__snapshots__/ChartBulletPrimarySegmentedMeasure.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartBullet/__snapshots__/ChartBulletQualitativeRange.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartContainer/__snapshots__/ChartContainer.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartCursorContainer/__snapshots__/ChartCursorContainer.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartCursorTooltip/__snapshots__/ChartCursorFlyout.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartCursorTooltip/__snapshots__/ChartCursorTooltip.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartDonut/__snapshots__/ChartDonut.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartDonutUtilization/__snapshots__/ChartDonutThreshold.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartDonutUtilization/__snapshots__/ChartDonutUtilization.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartGroup/__snapshots__/ChartGroup.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartLegend/__snapshots__/ChartLegend.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartLegendTooltip/__snapshots__/ChartLegendTooltip.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartLine/__snapshots__/ChartLine.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartPie/__snapshots__/ChartPie.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartPoint/__snapshots__/ChartPoint.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartScatter/__snapshots__/ChartScatter.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartStack/__snapshots__/ChartStack.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartThreshold/__snapshots__/ChartThreshold.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartTooltip/__snapshots__/ChartTooltip.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartVoronoiContainer/__snapshots__/ChartVoronoContainer.test.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
packages/react-charts/src/echarts/components/themes/colors/skeleton-theme.tspackages/react-charts/src/victory/components/ChartTheme/themes/base-theme.tspackages/react-charts/src/victory/components/ChartTheme/themes/colors/skeleton-theme.tspackages/react-charts/src/victory/components/ChartTheme/themes/skeleton-theme.tspackages/react-core/package.jsonpackages/react-docs/package.jsonpackages/react-icons/package.jsonpackages/react-styles/package.jsonpackages/react-tokens/package.json
lboehling
left a comment
lboehling
left a comment
There was a problem hiding this comment.
One small nit -- can we update the stroke around the bar charts to use border--color--alt instead of high-contrast border color? (regular & stacked -- i think they reference the same var, i just can't find it in the diff)
otherwise this all looks great to me!
|
@dlabrecq thanks! Everything is ready, let me know if you're trying to access something and it isn't working. |
4 participants
Relies on patternfly/patternfly#8379 as the core dependency to work
Updates charts with high contrast styles - it's the react half of patternfly/patternfly#7949
Has some hero component changes to pass the build that will need to be removed before merging.
Ran visual regressions for charts in light/dark/hc
Light
Light high contrast
Dark
Dark high contrast
Summary by CodeRabbit
Chores
Chart Improvements