-
Notifications
You must be signed in to change notification settings - Fork 81
[WIP] feat: add component config in ThemeProvider #365
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
Changes from all commits
52b5d63
342a65d
0003595
1ecbb60
80428be
ea95211
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import { getButtonSpacingProps } from '@coinbase/cds-common/utils/getButtonSpaci | |
| import { useTheme } from '../hooks/useTheme'; | ||
| import { Icon } from '../icons/Icon'; | ||
| import { Pressable, type PressableBaseProps } from '../system/Pressable'; | ||
| import { mergeComponentProps } from '../utils/mergeComponentProps'; | ||
|
|
||
| import type { ButtonBaseProps } from './Button'; | ||
|
|
||
|
|
@@ -27,26 +28,32 @@ export type IconButtonBaseProps = SharedProps & | |
|
|
||
| export type IconButtonProps = IconButtonBaseProps; | ||
|
|
||
| export const IconButton = memo(function IconButton({ | ||
| name, | ||
| active, | ||
| variant = 'secondary', | ||
| transparent, | ||
| compact = true, | ||
| background, | ||
| color, | ||
| borderColor, | ||
| borderWidth = 100, | ||
| borderRadius = 1000, | ||
| feedback = compact ? 'light' : 'normal', | ||
| flush, | ||
| loading, | ||
| style, | ||
| accessibilityHint, | ||
| accessibilityLabel, | ||
| ...props | ||
| }: IconButtonProps) { | ||
| export const IconButton = memo(function IconButton(_props: IconButtonProps) { | ||
| const theme = useTheme(); | ||
| const mergedProps = mergeComponentProps( | ||
| theme?.components?.IconButton, | ||
| _props, | ||
| theme?.components?.mergeStyleProps, | ||
| ); | ||
| const { | ||
| name, | ||
| active, | ||
| variant = 'secondary', | ||
| transparent, | ||
| compact = true, | ||
| background, | ||
| color, | ||
| borderColor, | ||
| borderWidth = 100, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| borderRadius = 1000, | ||
| feedback = compact ? 'light' : 'normal', | ||
| flush, | ||
| loading, | ||
| style, | ||
| accessibilityHint, | ||
| accessibilityLabel, | ||
| ...props | ||
| } = mergedProps; | ||
| const iconSize = compact ? 's' : 'm'; | ||
|
|
||
| const variantMap = transparent ? transparentVariants : variants; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,35 @@ | ||
| import type { TextStyle, ViewStyle } from 'react-native'; | ||
| import type { ColorScheme, ThemeVars } from '@coinbase/cds-common/core/theme'; | ||
|
|
||
| import type { ButtonBaseProps } from '../buttons/Button'; | ||
| import type { IconButtonBaseProps } from '../buttons/IconButton'; | ||
|
|
||
| type Shadow = { | ||
| shadowColor?: ViewStyle['shadowColor']; | ||
| shadowOpacity?: ViewStyle['shadowOpacity']; | ||
| shadowOffset?: ViewStyle['shadowOffset']; | ||
| shadowRadius?: ViewStyle['shadowRadius']; | ||
| }; | ||
|
|
||
| export type ComponentTheme = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. alternative to consider:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "variants" can't be themed e.g. Button "negative" can't be configured
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| Button: Partial<ButtonBaseProps>; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| IconButton: Partial<IconButtonBaseProps>; | ||
| /** | ||
| * Controls how component props from theme config are merged with local component props. | ||
| * @default false | ||
| * | ||
| * - When `false` (default): Local props simply override theme config props (standard object spread behavior). | ||
| * - When `true`: Special merging behavior for styling props: | ||
| * - `style`: Shallow merge (local props override theme config) | ||
| * - `styles`: Object keys merged, each value shallow merged | ||
| * - All other props: Local props override theme config | ||
| */ | ||
| mergeStyleProps?: boolean; | ||
|
stacysun-cb marked this conversation as resolved.
|
||
| }; | ||
| export type ComponentsConfig<Components = ComponentTheme> = { | ||
| [Key in keyof Components]?: Components[Key]; | ||
| }; | ||
|
|
||
| export type ThemeConfig = { | ||
| /** A unique identifier for the theme. */ | ||
| id?: string; | ||
|
|
@@ -48,6 +70,13 @@ export type ThemeConfig = { | |
| }; | ||
|
|
||
| export type Theme = ThemeConfig & { | ||
| /** | ||
| * Optional component configs at theme level. | ||
| * Allows configuring default props for specific components throughout the theme. | ||
| * These are merged with props passed directly to components, with local props taking precedence. | ||
| * Supports nested ThemeProvider inheritance with shallow merge. | ||
| */ | ||
| components?: ComponentsConfig; | ||
| /** The currently active color scheme for the parent ThemeProvider, either "light" or "dark". */ | ||
| activeColorScheme: ColorScheme; | ||
| /** The light or dark spectrum color values, as appropriate based on the activeColorScheme. */ | ||
|
|
||
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.
💡 maybe we could consider creating a higher order component to encapsulate and reuse prop merging across all our components