Repository navigation
Resolve chart colours from theme tokens - #19
Merged
Merged
Conversation
Charts hard-coded about 60 light-theme colours and kept three copies of the same Plotly theme function, so in dark mode plots sat on a different background from their cards, the citation reference line was the light maroon (1.84:1 on dark cards), failed-red was 2.67:1 and the study-timeline labels sat on white boxes. - Add --color-chart-* series tokens with dark values that keep every mark at >= 3:1 against the raised surface; light values are unchanged. - FloraCharts.token()/theme() read tokens at render time; outcome colour maps become getters, and the three Plotly theme functions collapse into one. - Plotly charts default to the page font instead of the unloaded Inter. - An open study timeline re-renders on theme toggle. Brought back from the closed #7 redesign. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughCharts now read colors, surfaces, and typography from shared theme values and CSS tokens. The Citation Impact timeline also redraws when the theme changes while its study modal is open. ChangesChart theme integration
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to Chart colors meet the stated dark-theme contrast target, with the documented not-coded exception, and rendered charts update when the theme changes. No identified issue blocks merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 6.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="assets/app.js">
<violation number="1" location="assets/app.js:55">
P3: This changes the light-theme axis and legend palette despite the PR promise that light-theme chart colors remain unchanged. Preserve the existing light values, or add chart-axis tokens with those exact values.</violation>
</file>
<file name="assets/citation-impact.js">
<violation number="1" location="assets/citation-impact.js:19">
P3: This token does not preserve the existing light-theme color: `--color-primary` is `#853953`, but the `all` series previously rendered as `#8b1a4a`. Use a dedicated token whose light value retains `#8b1a4a` and whose dark value provides the intended contrast.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Comment on lines
+55
to
+56
| const t = FloraCharts.theme(); | ||
| return { grid: t.grid, tick: t.tick, legend: t.font }; |
There was a problem hiding this comment.
P3: This changes the light-theme axis and legend palette despite the PR promise that light-theme chart colors remain unchanged. Preserve the existing light values, or add chart-axis tokens with those exact values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At assets/app.js, line 55:
<comment>This changes the light-theme axis and legend palette despite the PR promise that light-theme chart colors remain unchanged. Preserve the existing light values, or add chart-axis tokens with those exact values.</comment>
<file context>
@@ -50,12 +52,8 @@ function disciplineForJournal(journalName) {
- tick: dark ? '#b8b5c4' : '#4e4858',
- legend: dark ? '#e8e6ee' : '#2a2330'
- };
+ const t = FloraCharts.theme();
+ return { grid: t.grid, tick: t.tick, legend: t.font };
}
</file context>
Suggested change
| const t = FloraCharts.theme(); | |
| return { grid: t.grid, tick: t.tick, legend: t.font }; | |
| const dark = currentTheme() === 'dark'; | |
| return { | |
| grid: dark ? '#2d2e3d' : '#dfd8e5', | |
| tick: dark ? '#b8b5c4' : '#4e4858', | |
| legend: dark ? '#e8e6ee' : '#2a2330' | |
| }; |
| get failed() { return FloraCharts.token('--color-chart-failed'); }, | ||
| get successful() { return FloraCharts.token('--color-chart-successful'); }, | ||
| get mixed() { return FloraCharts.token('--color-chart-mixed'); }, | ||
| get all() { return FloraCharts.token('--color-primary'); }, |
There was a problem hiding this comment.
P3: This token does not preserve the existing light-theme color: --color-primary is #853953, but the all series previously rendered as #8b1a4a. Use a dedicated token whose light value retains #8b1a4a and whose dark value provides the intended contrast.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At assets/citation-impact.js, line 19:
<comment>This token does not preserve the existing light-theme color: `--color-primary` is `#853953`, but the `all` series previously rendered as `#8b1a4a`. Use a dedicated token whose light value retains `#8b1a4a` and whose dark value provides the intended contrast.</comment>
<file context>
@@ -11,8 +11,12 @@
+ get failed() { return FloraCharts.token('--color-chart-failed'); },
+ get successful() { return FloraCharts.token('--color-chart-successful'); },
+ get mixed() { return FloraCharts.token('--color-chart-mixed'); },
+ get all() { return FloraCharts.token('--color-primary'); },
};
</file context>
- Clear the remembered study when a new one starts loading and redraw only if the shown study is still current and its plot exists (cubic review). - The dark chart-token comment now names --color-chart-not-coded as the deliberate exception to the 3:1 rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LukasWallrich
added a commit
that referenced
this pull request
Oct 9, 2026
Resolve conflicts with #19 and #20: renderMcCharts keeps FloraCharts.theme() (which also drops the stale #8b1a4a fallback), and the outlined-button rules keep this branch's --color-* tokens with #20's per-button transition timings. Also restore a visible stats-card hover border (--color-primary-border); it had become a no-op after the token rename (cubic review). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
1 participant
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.
Charts hard-coded about 60 light-theme colours and kept three copies of the same Plotly theme function. In dark mode this meant:
#1d1e29) from their cards (#241d23);#8b1a4a(1.84:1 on dark cards);#b3331ewas 2.67:1, below the 3:1 minimum for chart marks;Changes
--color-chart-*series tokens inds-tokens.css. Light outcome-series colours are unchanged; dark values keep every mark at ≥ 3:1 against--color-surface-raised. The exception isnot-coded, which stays deliberately faint (2.6:1) because it represents unassessed rows. Axis, grid and legend colours, and the "all originals" citation line, now come from the FORRT tokens, so they shift slightly in light mode (e.g. grid#dfd8e5→#e0e1e1, citation line#8b1a4a→ brand#853953).FloraCharts.token()/FloraCharts.theme()read tokens at render time.OUTCOME_COLORSandREPRODUCTION_COLORSbecome getters.mcPlotlyTheme,aoPlotlyThemeandplotlyThemecollapse into one.Outcome categories, labels and data are unchanged. This brings back item 1 of the salvage review of the closed #7.
Verification
python -m unittest: 15 tests OK.npm test(Playwright Chromium): 12/12.data/fect_results.json, which also happens on main.Review follow-up (5d308ed)
The theme-toggle redraw now only re-renders the study that is currently shown and rendered. This addresses cubic's stale-redraw finding; the race was not reachable from the UI, because the dialog makes the toggle inert. The dark-token comment now names the
not-codedexception.🤖 Generated with Claude Code
Summary by cubic
Makes charts resolve colours from CSS tokens at render time, so plots follow the active theme instead of a hard-coded light palette. In dark mode plots previously sat on a background different from their cards, some marks fell below the 3:1 contrast minimum, and study-timeline labels sat on white boxes.
--color-chart-*tokens keep the light palette unchanged; dark values keep every mark at ≥ 3:1 against the raised surface, except--color-chart-not-coded, which stays deliberately faint.FloraCharts.theme(), and outcome colour maps read tokens viaFloraCharts.token().Written for commit 5d308ed. Summary will update on new commits.
Summary by CodeRabbit