Skip to content

Bump inactive chart toolbar contrast to match Line dropdown#2142

Merged
dcccrypto merged 1 commit intodcccrypto:mainfrom
0x-SquidSol:fix/chart-toolbar-contrast
May 7, 2026
Merged

Bump inactive chart toolbar contrast to match Line dropdown#2142
dcccrypto merged 1 commit intodcccrypto:mainfrom
0x-SquidSol:fix/chart-toolbar-contrast

Conversation

@0x-SquidSol
Copy link
Copy Markdown
Contributor

@0x-SquidSol 0x-SquidSol commented May 6, 2026

Summary

The Display dropdown's off-toggle rows, the f(x) menu's off-indicator labels, and the timeframe bar's inactive buttons were using `--text-dim` at rest — a token meant for placeholder-style metadata. On the dark chart bg the text reads as nearly illegible, requiring users to bump display brightness to scan options. ChartStyleMenu (Line) already uses `--text-secondary` for its non-selected rows; this PR aligns the other three controls to the same scheme.

What changes

Surface Before (rest / hover) After (rest / hover)
ChartStyleMenu (Line) — reference secondary / text + bg-surface unchanged
ChartDisplayMenu off rows dim / secondary, no bg secondary / text + bg-surface
ChartIndicatorMenu off labels dim / dim, no bg secondary / text + bg-surface (via row `group-hover:`)
ChartIndicatorMenu "Clear all" dim / text + bg-surface secondary / text + bg-surface
Timeframe buttons (inactive) dim / secondary, no bg secondary / text + bg-surface

Implementation notes

  • ChartIndicatorRow wraps in `group` + `hover:bg-[var(--bg-surface)]` and the label span uses `group-hover:text-[var(--text)]` so the row's hover state drives the label brightening. Cleaner than per-span hover handlers because the row also contains a colour swatch and per-config inputs when the indicator is enabled — those keep their own backgrounds (`bg-[var(--bg)]` on inputs, indicator-color on swatch) so they don't get washed out by the row tint.
  • Clear-all button kept its `disabled:opacity-40` fallback so 'no indicators yet' still reads visually disabled.
  • Active states untouched: timeframe stays accent-tinted, Display 'on' rows stay full-bright text, indicator 'on' labels stay full-bright.

Test plan

  • `vitest run` for ChartDisplayMenu and ChartIndicatorMenu — 36/36 passing
  • Manual smoke: open the chart, scan the toolbar — Display, f(x), timeframes should all read at the same brightness as the Line dropdown's options
  • Manual smoke: hover each off-state row — text should brighten and the row should pick up a subtle surface tint, identical to the Line dropdown's hover
  • Manual smoke: light theme — confirm new `--text-secondary` clears AA on `#FAFAFD` bg as expected (it already does for Line; this just extends the same pattern)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Style
    • Refined visual styling for chart overlay toggles with updated disabled text colors and enhanced hover highlighting.
    • Enhanced indicator menu styling with improved text color consistency and interactive hover effects.
    • Improved timeframe selector button interactions with updated hover state styling.

The Display dropdown rows, f(x) indicator labels, and timeframe
buttons were using --text-dim at rest — a token reserved for
placeholder-style metadata that doesn't clear WCAG AA on dark bg
for body text. Inactive interactable controls should sit at
--text-secondary instead, matching what ChartStyleMenu (Line)
already uses for its non-selected rows.

Hover affordance was also inconsistent: ChartStyleMenu rows
brighten text AND add a --bg-surface row highlight on hover, but
the other three controls only nudged the text from --text-dim to
--text-secondary, which left the hover state still dimmer than
the Line dropdown's resting state. All three now share one
contrast scheme:

  rest    → text-secondary
  hover   → text + bg-surface row highlight

ChartIndicatorRow uses a group / group-hover: pair on the
wrapping div + label span so the row's hover state drives the
label's brightening — cleaner than per-span hover handlers
because the row also contains a swatch and per-config inputs
when the indicator is enabled.

Clear-all button bumped from --text-dim to --text-secondary too;
the disabled:opacity-40 fallback still reads visually-disabled
when no indicators are active.
@0x-SquidSol 0x-SquidSol requested a review from dcccrypto as a code owner May 6, 2026 14:33
@vercel
Copy link
Copy Markdown

vercel Bot commented May 6, 2026

@0x-SquidSol is attempting to deploy a commit to the Khubair Nasir's projects Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 6, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2076e07b-d3aa-4777-9ef2-4c71ddb0c332

📥 Commits

Reviewing files that changed from the base of the PR and between 59c3f78 and 56004fa.

📒 Files selected for processing (3)
  • app/components/trade/ChartDisplayMenu.tsx
  • app/components/trade/ChartIndicatorMenu.tsx
  • app/components/trade/TradingChart.tsx

📝 Walkthrough

Walkthrough

Three trading chart UI components receive styling refinements: disabled text colors shift from dim to secondary variants, hover states gain background highlights for consistency, and group-hover contexts enable coordinated child element styling.

Changes

Trading Chart Styling Consistency

Layer / File(s) Summary
Color Token Updates
app/components/trade/ChartDisplayMenu.tsx, app/components/trade/ChartIndicatorMenu.tsx
Disabled text color changed from text-[var(--text-dim)] to text-[var(--text-secondary)] across overlay toggles and clear button.
Hover State Enhancements
app/components/trade/ChartDisplayMenu.tsx, app/components/trade/TradingChart.tsx
Hover backgrounds (hover:bg-[var(--bg-surface)]) and text colors (hover:text-[var(--text)]) added to overlay rows and timeframe buttons for visual consistency.
Group Styling Context
app/components/trade/ChartIndicatorMenu.tsx
group class added to ChartIndicatorRow container; indicator label styling converted to dynamic class array to enable group-hover text color transitions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • dcccrypto

Poem

🐰 A coat of fresh paint on the trading floor,
Colors hop brighter, hover some more,
Text-secondary gleams where dim once lay,
Backgrounds light up in the group-hover's sway,
Chart menus dressed up for a finer display!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: bumping the inactive chart toolbar contrast by updating color tokens from --text-dim to --text-secondary across multiple components to match the Line dropdown styling.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@dcccrypto dcccrypto merged commit 1ff16e1 into dcccrypto:main May 7, 2026
9 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants