UI: Increase border contrast of Checkbox, Radio, and Range #33064
Conversation
|
View your CI Pipeline Execution ↗ for commit f1c7e84
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughRefactored styling for three form controls: consolidated Range input track/thumb styles and state clusters; updated Checkbox and Radio inputs with explicit border, background, and border-radius changes plus revised disabled/checked state rules. Public APIs unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
Comment |
![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)
code/addons/docs/src/blocks/controls/Range.tsx (2)
27-35: Consider using strict equality for theme.base comparisons.Line 31 uses loose equality (
==) for the theme.base comparison. While functional, strict equality (===) is the recommended practice in TypeScript for type safety and consistency.Apply this diff:
- boxShadow: `${theme.base == 'dark' ? 'hsl(0 0 100 / 0.4)' : 'hsl(0 0 0 / 0.44)'} 0 0 0 1px inset`, + boxShadow: `${theme.base === 'dark' ? 'hsl(0 0 100 / 0.4)' : 'hsl(0 0 0 / 0.44)'} 0 0 0 1px inset`,
41-52: Consider using strict equality for theme.base comparisons.Line 48 uses loose equality (
==) for the theme.base comparison. Strict equality (===) is recommended for consistency with TypeScript best practices.Apply this diff:
- border: `1px solid ${theme.base == 'dark' ? 'hsl(0 0 100 / 0.4)' : 'hsl(0 0 0 / 0.44)'}`, + border: `1px solid ${theme.base === 'dark' ? 'hsl(0 0 100 / 0.4)' : 'hsl(0 0 0 / 0.44)'}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/addons/docs/src/blocks/controls/Range.tsx(1 hunks)code/core/src/components/components/Form/Checkbox.tsx(1 hunks)code/core/src/components/components/Form/Radio.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/components/components/Form/Checkbox.tsxcode/core/src/components/components/Form/Radio.tsx
🧬 Code graph analysis (2)
code/core/src/components/components/Form/Checkbox.tsx (1)
code/core/src/theming/base.ts (1)
color(1-42)
code/core/src/components/components/Form/Radio.tsx (1)
code/core/src/theming/base.ts (1)
color(1-42)
🔇 Additional comments (7)
code/addons/docs/src/blocks/controls/Range.tsx (2)
14-25: LGTM! Gradient computation centralized.The shared trackBackground constant consolidates the gradient logic for both light and dark themes, improving maintainability.
72-136: Excellent refactoring! Styling constants applied consistently.The use of shared styling constants (trackBaseStyles, thumbBaseStyles, etc.) across all vendor-specific selectors eliminates duplication and ensures consistent appearance. This significantly improves maintainability.
code/core/src/components/components/Form/Checkbox.tsx (3)
11-17: LGTM! Layout properties properly configured.The 14x14 size with grid layout and centered content is appropriate for the checkbox.
22-28: LGTM! Disabled state styling handles both unchecked and checked cases appropriately.The transparent background for disabled unchecked and theme-aware background for disabled checked/indeterminate states are properly implemented.
29-48: LGTM! Checked and focus states provide clear visual feedback.Removing the border when checked (line 30) and applying the secondary color background creates a clean, distinct appearance. The focus-visible outline maintains accessibility.
code/core/src/components/components/Form/Radio.tsx (2)
18-28: LGTM! Disabled state styling properly differentiates unchecked and checked states.The disabled state handling for both unchecked (transparent background) and checked (theme-aware background and border) states is correctly implemented.
29-37: LGTM! Checked state styling creates clear visual feedback.Setting both backgroundColor and borderColor to secondary (lines 30-31), combined with the inset box-shadow (line 32), creates the characteristic radio button appearance. The focus-visible outline maintains accessibility.
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Approving with code suggestions to apply for code formatting.
I think the dot on dark Range would look better if white than black (it's a lot of lines right now and those typically appear inside a table), but the goal was to fix contrast issues and they are fixed! The rest is your domain!
Co-authored-by: Steve Dodier-Lazaro <[email protected]>
Co-authored-by: Steve Dodier-Lazaro <[email protected]>
@Sidnioulz I agree, but I will refrain from redesigining it here. I do 100% want to take a solid pass at fixing this component up. Thanks for the review! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/components/components/Form/Checkbox.tsx (1)
8-8: Consider extracting border color values for maintainability.The hardcoded HSL values
hsl(0 0 100 / 0.4)andhsl(0 0 0 / 0.44)achieve the contrast goals but differ from the theme-token pattern used elsewhere (e.g., line 23'stheme.input.border). For consistency and maintainability, consider extracting these values to named constants or adding them to the theme.Example refactor:
+const CHECKBOX_BORDER_DARK = 'hsl(0 0 100 / 0.4)'; +const CHECKBOX_BORDER_LIGHT = 'hsl(0 0 0 / 0.44)'; + const Input = styled.input(({ theme }) => ({ appearance: 'none', backgroundColor: theme.input.background, - border: `1px solid ${theme.base === 'dark' ? 'hsl(0 0 100 / 0.4)' : 'hsl(0 0 0 / 0.44)'}`, + border: `1px solid ${theme.base === 'dark' ? CHECKBOX_BORDER_DARK : CHECKBOX_BORDER_LIGHT}`, borderRadius: 2,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/components/components/Form/Checkbox.tsx(1 hunks)code/core/src/components/components/Form/Radio.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/components/components/Form/Radio.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/components/components/Form/Checkbox.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/core/src/components/components/Form/Checkbox.tsx (5)
7-9: Previous issues addressed correctly.The strict equality operator and removal of commented code have been properly applied. The border styling now uses precise HSL values to achieve the contrast improvements stated in the PR objectives.
12-16: LGTM!The sizing (14px) and centering properties are appropriate for the checkbox layout.
21-24: LGTM!The disabled state styling appropriately uses
transparentbackground and theme-based border color for visual consistency.
25-27: LGTM!The disabled checked/indeterminate state correctly uses theme-based colors to maintain visual hierarchy in disabled states.
28-31: LGTM!Removing the border in checked/indeterminate states while using a distinct background color is an effective pattern. The visual contrast is maintained through the secondary color background and the checkmark/dash indicators.
valentinpalkovic
added
the
needs qa
yannbf
removed
the
needs qa
Closes #
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This pull request has been released as version
0.0.0-pr-33064-sha-f1c7e84a. Try it out in a new sandbox by runningnpx [email protected] sandboxor in an existing project withnpx [email protected] upgrade.More information
0.0.0-pr-33064-sha-f1c7e84am/update-styles-checkbox-radio-rangef1c7e84a1763997669)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=33064Summary by CodeRabbit
Style
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.