-
Notifications
You must be signed in to change notification settings - Fork 41
[V2] Colour Sheet from Invision wireframes + added to Button + stories #1632
Conversation
@@ -28,7 +28,7 @@ export const ThemeToggler = (DecoratedStory: () => JSX.Element): JSX.Element => | |||
return ( | |||
<> | |||
<ThemeProvider theme={theme}> | |||
<Frame style={{ backgroundColor: darkMode ? COLOURS.bgDark : COLOURS.bgLight }}>{DecoratedStory()}</Frame> | |||
<Frame style={{ background: darkMode ? COLOURS.bgDark : COLOURS.bgLight }}>{DecoratedStory()}</Frame> |
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 it an idea to further abstract this away into a 'setMode' type of hook? So that the hook determines what mode we're on and returns either COLOURS.bgDark
or COLOURS.bgLight
?
Down the road perhaps we can even further abstract it down to:
styles.backgroundDefault
which first conditionally determines what mode we're on and then only return the bg color.
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.
Down the road perhaps we can even further abstract it down to:
styles.backgroundDefault which first conditionally determines what mode we're on and then only return the bg color.
how would you do this? styles
is in another file and should remain there. To conditionally set it there wouldn't be possible unless it used some logic elsewhere or within a component.
The hook approach is imo the only viable solution, or at least the most straight forward without spending too much time. The user's selection would then be saved to localstorage and loaded on app load (as we do now in v1)
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.
for now this is just a lazy man's version of creating a decorator i needed for testing, it will be extrapolated into a hook down the line (sth to check in v1 if we already have, which i believe we do and can repurpose)
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 was thinking something along the lines of a hook. Assuming we can call that from within the styles file context. So styles would only return you stuff, after checking the current mode with hooks.
padding: 0.65rem 1rem; | ||
} | ||
*/ | ||
|
||
export default styled.button` | ||
border: ${buttonBorder}; | ||
|
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.
Can this whole StyledButton.ts be kept dryer so that instead doing:
warning: {
light: css`
color: ${white};
background: ${warningLight};
&:hover {
background: ${warningDark};
border-color: ${warningDark};
}
`,
dark: css`
color: ${white};
background: ${warningDark};
&:hover {
background: ${warningLight};
border-color: ${warningLight};
}
`,
},
doing
warning: {
css`
color: ${white};
background: ${warningBgDefault};
&:hover {
background: ${warningBgHoverDefault};
border-color: ${warningBorderColorDefault};
}
`
},
Then e.g. we could have a variable warningDefault
(the above vars are a bit verbose, but for the sake of demo-ing) which conditionally returns a color, based on getting the current mode (either dark or light).
This keeps this styling code dryer for defaults. For overrides we can specifically override the colors in a individual component. Let me know your thoughts.
|
||
export interface ButtonBaseProps extends React.ButtonHTMLAttributes<Element> { | ||
kind?: keyof typeof ButtonVariations | ||
} | ||
|
||
export const ButtonBase = styled(StyledButton)<ButtonBaseProps>` | ||
border-radius: 2rem; | ||
border-radius: ${styles.borderRadius}; |
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.
why do we want to import this from the styles?
Is because we expect to use always the same border radius and u want to have it in a constant?
I mean, if we really want to all have the same value, ok, but I think the more independent the components are the better. I do see, that if we repeat ourselves a lot, we can have some fragments we reuse, just not sure this is the case.
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.
So in the context of a Button
in my view it makes sense to use a default global value for the borderRadius
. It is to be assumed that we'd use at most 2 distinct values for the border-radius. Where a lot of them will simply use a default.
This applies to other styled properties too.
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.
@anxolin I think for buttons this will and should be the case. For others, I 100% agree we should be more granular and create fragments where we need. In this case I believe it should remain this way, for consistency in the buttons
* added styled-theming lib * styling sheets 1. colours 2. styles 3. commented out stuff in theme * simplify ButtonBase code + add stories * adjust darkMode decorator for new theme 1, add colour to colourSheet * StyledButton base * addressed PR comments: 1. added type to Story 2. Changed DarkThemeTOggler to ThemeToggler 3. simplified ButtonVariant enum * fix button type, add disabled button, string enum 1. use string enum as button type 2. default to dark mode on start * [V2] Colour Sheet from Invision wireframes + added to Button + stories (#1632) * cleanup button story * new colour sheet * fix type, remove class styles, use const style * use new colours in StyledButton * misc. 1. add borderRadius to const 2. use theme bg for darkMode switch * [V2] Button sizes (#1633) * wrap our base styled button in own theme provider and add size opt * add proper types for button size * Stories: add B I G and smol button stories * misc. 1. make theme toggle button smol 2. add buttonFontSize to styles sheet * enum > type for ButtonVariations * incorrect darkMode button logo * better prop names (#1637) 1. kind > $type 2. size > $size * [V2 button] Rearrange component file structure and small prop fixes (#1640) * saving * $transient name props not allowed - use _propName * change props again * _type > variation * _size > size * variation > variant prop name
* creating theme pt 1 * theme * DarkModeThemeToggler decorator * Button common component * Button story * cleaned up Theme types * simplify ButtonBase prop logic * unmemo fn * fix story bprops * V2/button theming (#1626) * added styled-theming lib * styling sheets 1. colours 2. styles 3. commented out stuff in theme * simplify ButtonBase code + add stories * adjust darkMode decorator for new theme 1, add colour to colourSheet * StyledButton base * addressed PR comments: 1. added type to Story 2. Changed DarkThemeTOggler to ThemeToggler 3. simplified ButtonVariant enum * fix button type, add disabled button, string enum 1. use string enum as button type 2. default to dark mode on start * [V2] Colour Sheet from Invision wireframes + added to Button + stories (#1632) * cleanup button story * new colour sheet * fix type, remove class styles, use const style * use new colours in StyledButton * misc. 1. add borderRadius to const 2. use theme bg for darkMode switch * [V2] Button sizes (#1633) * wrap our base styled button in own theme provider and add size opt * add proper types for button size * Stories: add B I G and smol button stories * misc. 1. make theme toggle button smol 2. add buttonFontSize to styles sheet * enum > type for ButtonVariations * incorrect darkMode button logo * better prop names (#1637) 1. kind > $type 2. size > $size * [V2 button] Rearrange component file structure and small prop fixes (#1640) * saving * $transient name props not allowed - use _propName * change props again * _type > variation * _size > size * variation > variant prop name
* creating theme pt 1 * theme * DarkModeThemeToggler decorator * Button common component * Button story * cleaned up Theme types * simplify ButtonBase prop logic * unmemo fn * fix story bprops * V2/button theming (#1626) * added styled-theming lib * styling sheets 1. colours 2. styles 3. commented out stuff in theme * simplify ButtonBase code + add stories * adjust darkMode decorator for new theme 1, add colour to colourSheet * StyledButton base * addressed PR comments: 1. added type to Story 2. Changed DarkThemeTOggler to ThemeToggler 3. simplified ButtonVariant enum * fix button type, add disabled button, string enum 1. use string enum as button type 2. default to dark mode on start * [V2] Colour Sheet from Invision wireframes + added to Button + stories (#1632) * cleanup button story * new colour sheet * fix type, remove class styles, use const style * use new colours in StyledButton * misc. 1. add borderRadius to const 2. use theme bg for darkMode switch * [V2] Button sizes (#1633) * wrap our base styled button in own theme provider and add size opt * add proper types for button size * Stories: add B I G and smol button stories * misc. 1. make theme toggle button smol 2. add buttonFontSize to styles sheet * enum > type for ButtonVariations * incorrect darkMode button logo * better prop names (#1637) 1. kind > $type 2. size > $size * [V2 button] Rearrange component file structure and small prop fixes (#1640) * saving * $transient name props not allowed - use _propName * change props again * _type > variation * _size > size * variation > variant prop name * add polished, css interpolation pkg
馃挧 into #1626
V2/Colour Sheet from Invision wireframes + added to Button + stories
Button stories use new colours, + small optimisation to button/styledButton base code