-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to theme
prop proposal
#1085
Update to theme
prop proposal
#1085
Conversation
|
CustomTheme.args = { | ||
theme: { | ||
backgroundUnselected: 'black', | ||
backgroundSelected: 'hotpink', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the theme: choiceCardDark
better if we consider this as part of the documentation for this component. It is the "recommended" way of theming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i think it would be if we had a dark theme for this component. in that case we'd want to explicit provide a story for that (and any other built-in themes).
the reason i removed it here is because we don't (yet?) have that, and so hopefully this makes clear it's a made up example of one way of using the prop, rather than implying there was a dark theme you can use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Happy to go with that, we could change the colours to come from the palette because that may be more how we want the prop used. I am happy to do that once this is merged tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i think we do want that, but part of me felt that because right now you can do the 'wrong' thing, we should have a story that does that, then have it break when we fix it. maybe that's the wrong approach.
but maybe we shoudl fix the types in foundations so you can't?
libs/@guardian/source-react-components/src/choice-card/ChoiceCard.tsx
Outdated
Show resolved
Hide resolved
…ard.tsx Co-authored-by: Oliver Abrahams <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Oliver Abrahams <[email protected]>
What are you changing?
choiceCardTheme
type