fix: use light foreground color on live stream page for spectrogram#2863
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA CSS styling update to the start button in the LiveStreamPage component, changing the text color class from using Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Code Review
This pull request updates the styling of the start button on the Live Stream page, changing the text color and hover state to use white instead of theme variables. A review comment suggests refining the hover effect by using the "enabled:" modifier to prevent the hover style from appearing when the button is disabled, ensuring better visual feedback for users.
| onclick={handleStartClick} | ||
| disabled={!selectedSourceId || sources.length === 0} | ||
| class="flex h-full w-full flex-col items-center justify-center gap-3 bg-black text-[var(--color-base-content)]/60 transition-colors hover:text-[var(--color-base-content)]/80 disabled:cursor-not-allowed disabled:opacity-50" | ||
| class="flex h-full w-full flex-col items-center justify-center gap-3 bg-black text-white/75 transition-colors hover:text-white disabled:cursor-not-allowed disabled:opacity-50" |
There was a problem hiding this comment.
The hover:text-white style will still trigger when the button is disabled, which can be confusing for users as it suggests interactivity. It's better to restrict the hover effect to the enabled state using the enabled: modifier.
class="flex h-full w-full flex-col items-center justify-center gap-3 bg-black text-white/75 transition-colors enabled:hover:text-white disabled:cursor-not-allowed disabled:opacity-50"
|
Nice catch, thanks for fixing this. The white text on the black spectrogram background is the right call here. |
On the live stream page, if you're using light mode, it is very hard to see the play button/"Audio Source" text. See below for comparison.
This PR standardizes on light foreground color for this spectrogram, since it's overlayed on a black background
Summary by CodeRabbit