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 Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
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
| import chart_legend_data_stroke_Color from '@patternfly/react-tokens/dist/esm/chart_legend_data_stroke_Color'; | ||
| import chart_legend_data_stroke_Width from '@patternfly/react-tokens/dist/esm/chart_legend_data_stroke_Width'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the 6 new `@patternfly/react-tokens` ESM token files exist in the monorepo.
NEW_TOKENS=(
"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"
)
for token in "${NEW_TOKENS[@]}"; do
result=$(fd --type f "${token}" --extension js --extension ts)
if [ -z "$result" ]; then
echo "MISSING: $token"
else
echo "FOUND: $result"
fi
doneRepository: patternfly/patternfly-react
Length of output: 315
Add missing token files to @patternfly/react-tokens or remove their imports.
All six token imports at lines 60–61, 104–105, and 120–121 reference non-existent token module files in the monorepo:
chart_legend_data_stroke_Colorchart_legend_data_stroke_Widthchart_donut_pie_data_stroke_Colorchart_donut_pie_data_stroke_Widthchart_bullet_bar_stroke_Colorchart_bullet_bar_stroke_Width
These missing imports will cause build failures when resolving the module paths. Either add the token files to the @patternfly/react-tokens package or remove these imports from the theme file.
🤖 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 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.
2 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 theme - backstop-light.pdf
Dark theme - backstop-dark.pdf
High contrast - backstop-hc.pdf
High contrast dark - backstop-dark-hc.pdf
If you want to see the actual reports so you can open the images and hwhat-not, here's a zip. Just open the "index.html" files in the "html_report" directories - https://drive.google.com/file/d/1Hu9toF55uln05UzqXi2wvLyxP8DcSFRa/view?usp=sharing
Summary by CodeRabbit
Chores
Chart Improvements