feat(charts): add high contrast - #8379
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds high-contrast chart token files and Red Hat high-contrast tokens, refactors chart label stroke into separate color and width CSS custom properties, updates chart SCSS to include and use high-contrast tokens, and regenerates timestamps in many token files. ChangesHigh-Contrast Chart Token Support
Sequence Diagram(s)sequenceDiagram
participant DevPipeline
participant TokenFiles
participant ThemeIncludes
participant ChartStyles
DevPipeline->>TokenFiles: generate/add high-contrast token mixins
TokenFiles->>ThemeIncludes: expose token mixins via `@use`
ThemeIncludes->>ChartStyles: include tokens under theme selectors
ChartStyles->>ChartStyles: update per-chart CSS custom properties to use HC tokens
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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-pr-8379.surge.sh A11y report: https://pf-pr-8379-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: 3
🧹 Nitpick comments (1)
src/patternfly/patternfly-charts.scss (1)
108-110: ⚡ Quick winWire label stroke width to the new token
You’ve split label stroke into color/width tokens, but width is still hardcoded (
0) here, so theme-level width overrides can’t take effect.Proposed fix
- --#{$chart}-global--label--stroke: var(--pf-t--chart--global--label--stroke--color); + --#{$chart}-global--label--stroke: var(--pf-t--chart--global--label--stroke--color); --#{$chart}-global--label--text-anchor: var(--pf-t--chart--global--label--text-anchor); - --#{$chart}-global--label--stroke--Width: 0; + --#{$chart}-global--label--stroke--Width: var(--pf-t--chart--global--label--stroke--width);🤖 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 `@src/patternfly/patternfly-charts.scss` around lines 108 - 110, The label stroke width is hardcoded to 0 and uses a capitalized property name (--#{$chart}-global--label--stroke--Width), so theme overrides can't apply; change the property to use a lowercase suffix and wire it to the new token, i.e. replace the hardcoded line with --#{$chart}-global--label--stroke--width: var(--pf-t--chart--global--label--stroke--width); so the theme-level width token is respected (update the property name and its value to the token).
🤖 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 `@src/patternfly/base/tokens/tokens-redhat-highcontrast.scss`:
- Line 732: The token mapping for
--pf-t--global--text--color--status--on-danger--clicked incorrectly references
the icon inverse token (--pf-t--global--icon--color--inverse); update that
mapping to use the text inverse token (--pf-t--global--text--color--inverse) so
it matches neighboring text mappings and the source token semantics, then
regenerate.
- Around line 64-66: The lint failures come from capitalized generic font family
keywords in the generated tokens (--pf-t--global--font--family--100,
--pf-t--global--font--family--200, --pf-t--global--font--family--300); fix by
normalizing generic family names to lowercase in the token generator/source so
it emits "helvetica, arial, sans-serif, courier, monospace" (or update the
generator's fallback mapping) and regenerate tokens, or alternatively update CI
linting to ignore this generated file (exclude it from the stylelint
keyword-case rule) so the produced values for those token variables no longer
trigger keyword-case errors.
- Line 598: There are duplicate declarations for the custom property
--pf-t--global--color--status--read--on-secondary (one at the earlier occurrence
and another later that overrides it); update the token generator/source mapping
that emits tokens-redhat-highcontrast.scss so it emits a single canonical value
for --pf-t--global--color--status--read--on-secondary (and ensure
--pf-t--global--color--status--read--on-primary remains correct), by removing
the duplicate mapping or consolidating conflicting token inputs in the generator
so only one declaration for that custom property is produced.
---
Nitpick comments:
In `@src/patternfly/patternfly-charts.scss`:
- Around line 108-110: The label stroke width is hardcoded to 0 and uses a
capitalized property name (--#{$chart}-global--label--stroke--Width), so theme
overrides can't apply; change the property to use a lowercase suffix and wire it
to the new token, i.e. replace the hardcoded line with
--#{$chart}-global--label--stroke--width:
var(--pf-t--chart--global--label--stroke--width); so the theme-level width token
is respected (update the property name and its value to the token).
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9ef2e3ae-90a5-43c1-bc80-be7c1ddb1939
📒 Files selected for processing (17)
src/patternfly/base/tokens/tokens-charts-dark.scsssrc/patternfly/base/tokens/tokens-charts-highcontrast-dark.scsssrc/patternfly/base/tokens/tokens-charts-highcontrast.scsssrc/patternfly/base/tokens/tokens-charts.scsssrc/patternfly/base/tokens/tokens-dark.scsssrc/patternfly/base/tokens/tokens-default.scsssrc/patternfly/base/tokens/tokens-felt-dark.scsssrc/patternfly/base/tokens/tokens-felt-glass-dark.scsssrc/patternfly/base/tokens/tokens-felt-glass.scsssrc/patternfly/base/tokens/tokens-felt-highcontrast-dark.scsssrc/patternfly/base/tokens/tokens-felt-highcontrast.scsssrc/patternfly/base/tokens/tokens-felt.scsssrc/patternfly/base/tokens/tokens-glass-dark.scsssrc/patternfly/base/tokens/tokens-glass.scsssrc/patternfly/base/tokens/tokens-palette.scsssrc/patternfly/base/tokens/tokens-redhat-highcontrast.scsssrc/patternfly/patternfly-charts.scss
| --pf-t--global--font--family--100: "Red Hat Text", "RedHatText", "Noto Sans Arabic", "Noto Sans Hebrew", "Noto Sans JP", "Noto Sans KR", "Noto Sans Malayalam", "Noto Sans SC", "Noto Sans TC", "Noto Sans Thai", Helvetica, Arial, sans-serif; | ||
| --pf-t--global--font--family--200: "Red Hat Display", "RedHatDisplay", "Noto Sans Arabic", "Noto Sans Hebrew", "Noto Sans JP", "Noto Sans KR", "Noto Sans Malayalam", "Noto Sans SC", "Noto Sans TC", "Noto Sans Thai", Helvetica, Arial, sans-serif; | ||
| --pf-t--global--font--family--300: "Red Hat Mono", "RedHatMono", "Courier New", Courier, monospace; |
There was a problem hiding this comment.
Resolve stylelint keyword-case failures in generated font-family fallbacks
Static analysis flags Line 64-Line 66 for value keyword casing (Helvetica/Arial/Courier). If this generated file is linted, it can block CI.
Since this file is generated, either normalize casing in the token generator/source or exclude generated token artifacts from this lint rule.
🧰 Tools
🪛 Stylelint (17.10.0)
[error] 64-64: Expected "Helvetica" to be "helvetica" (value-keyword-case)
(value-keyword-case)
[error] 64-64: Expected "Arial" to be "arial" (value-keyword-case)
(value-keyword-case)
[error] 65-65: Expected "Helvetica" to be "helvetica" (value-keyword-case)
(value-keyword-case)
[error] 65-65: Expected "Arial" to be "arial" (value-keyword-case)
(value-keyword-case)
[error] 66-66: Expected "Courier" to be "courier" (value-keyword-case)
(value-keyword-case)
🤖 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 `@src/patternfly/base/tokens/tokens-redhat-highcontrast.scss` around lines 64 -
66, The lint failures come from capitalized generic font family keywords in the
generated tokens (--pf-t--global--font--family--100,
--pf-t--global--font--family--200, --pf-t--global--font--family--300); fix by
normalizing generic family names to lowercase in the token generator/source so
it emits "helvetica, arial, sans-serif, courier, monospace" (or update the
generator's fallback mapping) and regenerate tokens, or alternatively update CI
linting to ignore this generated file (exclude it from the stylelint
keyword-case rule) so the produced values for those token variables no longer
trigger keyword-case errors.
| --pf-t--global--color--brand--accent--clicked: var(--pf-t--global--color--brand--clicked); | ||
| --pf-t--global--color--brand--accent--default: var(--pf-t--global--color--brand--default); | ||
| --pf-t--global--color--brand--accent--hover: var(--pf-t--global--color--brand--hover); | ||
| --pf-t--global--color--status--read--on-primary: var(--pf-t--global--background--color--secondary--default); |
There was a problem hiding this comment.
Remove duplicate declaration for --pf-t--global--color--status--read--on-secondary
Line 598 and Line 754 define the same custom property with different values; Line 754 silently overrides Line 598. This creates ambiguous intent and can cause theme drift.
Because this file is generated, please fix the source token mapping/generator so only one canonical value is emitted.
Also applies to: 754-754
🤖 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 `@src/patternfly/base/tokens/tokens-redhat-highcontrast.scss` at line 598,
There are duplicate declarations for the custom property
--pf-t--global--color--status--read--on-secondary (one at the earlier occurrence
and another later that overrides it); update the token generator/source mapping
that emits tokens-redhat-highcontrast.scss so it emits a single canonical value
for --pf-t--global--color--status--read--on-secondary (and ensure
--pf-t--global--color--status--read--on-primary remains correct), by removing
the duplicate mapping or consolidating conflicting token inputs in the generator
so only one declaration for that custom property is produced.
| --pf-t--global--text--color--status--on-custom--clicked: var(--pf-t--global--text--color--inverse); | ||
| --pf-t--global--text--color--status--on-custom--default: var(--pf-t--global--text--color--inverse); | ||
| --pf-t--global--text--color--status--on-custom--hover: var(--pf-t--global--text--color--inverse); | ||
| --pf-t--global--text--color--status--on-danger--clicked: var(--pf-t--global--icon--color--inverse); |
There was a problem hiding this comment.
Use the text inverse token for the text color mapping
Line 732 maps --pf-t--global--text--color--status--on-danger--clicked to --pf-t--global--icon--color--inverse, while neighboring text mappings use --pf-t--global--text--color--inverse. This is a semantic mismatch and may break contrast behavior if icon/text inverse tokens diverge.
Please align this to the text token in the source token definition before regenerating.
🤖 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 `@src/patternfly/base/tokens/tokens-redhat-highcontrast.scss` at line 732, The
token mapping for --pf-t--global--text--color--status--on-danger--clicked
incorrectly references the icon inverse token
(--pf-t--global--icon--color--inverse); update that mapping to use the text
inverse token (--pf-t--global--text--color--inverse) so it matches neighboring
text mappings and the source token semantics, then regenerate.
|
Thumbs up from @lboehling on this initial pass by testing in react and sifting through visual regressions of charts in light/dark/hc-light/hc-dark. These are needed to create a react PR so going ahead and merging. |
|
🎉 This PR is included in version 6.5.0-prerelease.86 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
fixes #7949
Relies on token updates from patternfly/design-tokens#148
Used to do the react work in patternfly/patternfly-react#12419
Summary by CodeRabbit
New Features
Updates