feat(checkbox): Add theming support #456
Conversation
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
| * | ||
| * @returns {CSSObject} the css object for the focus ring style | ||
| */ | ||
| export function themedFocusRing( |
There was a problem hiding this comment.
Interested to hear thoughts on this. I did this for a few reasons:
-
Unfortunately because we are usually calling this from within a class component, we can't use
React.useContext. This means we need to pass thethemeinto thefocusRingfunction. It would have been pretty nasty passing in every prop just to getthemeat the end. -
I'm often frustrated by the unclear nature of these params when you see them in use. The added complexity of an
optionsobject was worth it for the legibility and possibility for future changes IMO. This now looks likethemedFocusRing(theme, {separation: 2, animate: false})instead offocusRing(2, 2, false). I realize IDE hinting could help, but I still prefer the options object. -
Rather than trying to hack the existing
focusRingfunction to accept individual params vs. an options object without a breaking change, I figured a new function would be cleaner. We can consider deprecatingfocusRingin the future.
There was a problem hiding this comment.
I'm fine with this change. The theme context can be passed either through the Hooks API or the Context API. I think this function shouldn't be burdened to care how theme gets here.
| </ComponentStatesTable> | ||
| </StaticStates> | ||
| )); | ||
| .add('States', () => <CheckboxStates />) |
There was a problem hiding this comment.
Story source won't be very useful for these stories, but that is probably okay because we don't expect people to use the source code for these visual stories.
There was a problem hiding this comment.
yep yep, I think we pulled this out into a variable because StaticStates won't make much sense to someone viewing the source code.
Summary
We're adding theming to our inputs. The consumer must wrap their components in a
CanvasProviderand then they can pass their custom theme to any child components through thethemeprop.Partially addresses #358
Checklist
yarn testpasses