[WIP] feat: add component config in ThemeProvider#365
[WIP] feat: add component config in ThemeProvider#365stacysun-cb wants to merge 6 commits intomasterfrom
Conversation
🟡 Heimdall Review Status
🟡
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
🟡
0/1
|
Denominator calculation
|
stacysun-cb
had a problem deploying
to
production
GitHub Actions
Error
stacysun-cb
force-pushed
the
stacy/add-component-config
branch
from
5769e96 to
0003595
Compare
stacysun-cb
had a problem deploying
to
production
GitHub Actions
Error
stacysun-cb
had a problem deploying
to
production
GitHub Actions
Error
stacysun-cb
temporarily deployed
to
production
GitHub Actions
Inactive
| { | ||
| forwardRef(function Button(_props: ButtonProps, ref: React.ForwardedRef<View>) { | ||
| const theme = useTheme(); | ||
| const mergedProps = mergeComponentProps( |
There was a problem hiding this comment.
💡 maybe we could consider creating a higher order component to encapsulate and reuse prop merging across all our components
| background, | ||
| color, | ||
| borderColor, | ||
| borderWidth = 100, |
There was a problem hiding this comment.
default values like this should probably be encapsulated in our "default" component-level theme right? I know we're not going for 100% coverage when we launch this feature, but this is the type of thing it is being introduce for, right?
| }; | ||
|
|
||
| export type ComponentTheme = { | ||
| Button: Partial<ButtonBaseProps>; |
There was a problem hiding this comment.
we will want to be more strict than ever around what goes into base props vs full props. Maybe time for us to holistically examine that pattern together
| shadowRadius?: ViewStyle['shadowRadius']; | ||
| }; | ||
|
|
||
| export type ComponentTheme = { |
There was a problem hiding this comment.
alternative to consider:
Button: Pick<StyleProps, "borderRadius" | "padding" | "paddingHorizontal" | etc.> & {
className?: string;
style?: CSSProperties;
}
// this limits customers to only overriding style opinions, keeping the theme oriented around the theme
const customTheme = {
// ...
Button: {
borderRadius: 900 // can only use theme tokens
}
}
// prevents abuse from using theme to control behavior
// ...
Button: {
onClick: () = {} // shouldn't support this
variant: "negative" // doesn't make sense to set this globally
}
There was a problem hiding this comment.
ButtonBaseProps <> StyleProps
There was a problem hiding this comment.
- stronger semantics
- easier to add more harder to take away
There was a problem hiding this comment.
"variants" can't be themed e.g. Button "negative" can't be configured
There was a problem hiding this comment.
are some "variant" props an anti-pattern? Do they leak coinbase design patterns to open source i.e we are potentially over opinionated than we need to be and customers can achieve patterns/variants by settings props
stacysun-cb
temporarily deployed
to
production
GitHub Actions
Inactive
3 participants
What changed? Why?
Root cause (required for bugfixes)
UI changes
Testing
How has it been tested?
Testing instructions
Illustrations/Icons Checklist
Required if this PR changes files under
packages/illustrations/**orpackages/icons/**Change management
type=routine
risk=low
impact=sev5
automerge=false