[V2] common button component + stories #1624
Conversation
| border-color: ${({ button }): string => button.bg1Hover}; | ||
| background-color: ${({ button }): string => button.bg1Hover}; | ||
| color: ${({ button }): string => button.text1Hover}; | ||
| } | ||
|
|
||
| border-radius: 2rem; | ||
| cursor: pointer; | ||
| font-weight: bold; | ||
| outline: 0; | ||
| padding: 0.5rem 1rem; | ||
|
|
||
| transition: all 0.2s ease-in-out; | ||
| transition-property: color, background-color, border-color, opacity; | ||
|
|
||
| &.cancel { | ||
| background: transparent; | ||
| color: var(--color-text-active); |
There was a problem hiding this comment.
I'm really not a fan of how we mix styled-comp theme and css vars theme, but whatever
There was a problem hiding this comment.
Me neither, will rework for prop based logic 🤙
There was a problem hiding this comment.
agree. In v2 we could use only the theme
src/storybook/decorators.tsx
Outdated
| const [darkMode, setDarkMode] = React.useState(false) | ||
| const theme = colors(darkMode) | ||
|
|
||
| const handleDarkMode = React.useCallback(() => setDarkMode((darkMode) => !darkMode), []) |
There was a problem hiding this comment.
Wanna extract this into a hook?
|
@Velenir thanks, will review. Thoughts on theme data structure? Feels like it could be optimised |
| border-color: ${({ button }): string => button.bg1Hover}; | ||
| background-color: ${({ button }): string => button.bg1Hover}; | ||
| color: ${({ button }): string => button.text1Hover}; | ||
| } | ||
|
|
||
| border-radius: 2rem; | ||
| cursor: pointer; | ||
| font-weight: bold; | ||
| outline: 0; | ||
| padding: 0.5rem 1rem; | ||
|
|
||
| transition: all 0.2s ease-in-out; | ||
| transition-property: color, background-color, border-color, opacity; | ||
|
|
||
| &.cancel { | ||
| background: transparent; | ||
| color: var(--color-text-active); |
There was a problem hiding this comment.
agree. In v2 we could use only the theme
| // $alt button theme toggle | ||
| $alt?: boolean | ||
| $border?: boolean | ||
| theme: ThemedStyledProps<ColourTheme, ColourTheme> |
There was a problem hiding this comment.
Why are we injecting explicitly the Theme instead of using Uniswap approach of
declare module 'styled-components' {
export interface DefaultTheme extends Colors {
}
}
This way, all components have access to the theme and we don't need to add it on every prop decoration. They are part of their context.
| export default { | ||
| title: 'Common/Button', | ||
| component: ButtonBase, | ||
| decorators: [DarkModeThemeToggler], |
There was a problem hiding this comment.
Can we call it just ThemeToggler?
| return ( | ||
| <> | ||
| <ThemeProvider theme={theme}> | ||
| <Frame style={{ backgroundColor: 'lightgray' }}>{DecoratedStory()}</Frame> |
There was a problem hiding this comment.
I wouldn't add this gray frame.
I think, we can add a second decorator for the frame and leave the default color.
| const handleDarkMode = (): void => setDarkMode((darkMode) => !darkMode) | ||
|
|
||
| return ( | ||
| <> |
There was a problem hiding this comment.
I think the background of the whole story should be the background of the app
| return theme | ||
| } | ||
|
|
||
| // FOR REFERENCE |
There was a problem hiding this comment.
ok, but this is from v1?
not sure if we will need it even as a reference. We have the colors in the original design.
what do you think @biocom
There was a problem hiding this comment.
For the trading interface, I currently have these colors/variables defined here https://github.com/gnosis/dex-react/blob/0f9feb35788877e80aebbfe3027a8b56f3afbe83/src/components/layout/GenericLayout/variablesCss.ts
There was a problem hiding this comment.
Not sure if we want to consolidate theming between Simple Swap <-> Trading Interface. But I'd say in terms of colors they likely 90% overlap.
|
closing in favour for the BIGGER, the BETTER, the SEXIER, #1626 and co. |
Start of theming with styled-components and common button component aimed at reuse
Would love feedback on data structure of theme object