Solidify button appearance combos, document active state #2
Conversation
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@storybook/design-system", | |||
| "version": "0.0.10", | |||
| "version": "0.0.11", | |||
There was a problem hiding this comment.
I would like to pull this into the learn storybook repo, so I think it needs another release
| @@ -1,6 +1,6 @@ | |||
| import React, { Fragment } from 'react'; | |||
| import PropTypes from 'prop-types'; | |||
| import styled, { css } from 'styled-components'; | |||
| import styled from 'styled-components'; | |||
There was a problem hiding this comment.
As mentioned, these css calls are not necessary in this context
| @@ -441,16 +464,17 @@ Button.propTypes = { | |||
| Buttons with icons by themselves have a circular shape | |||
| */ | |||
| containsIcon: PropTypes.bool, | |||
| size: PropTypes.oneOf(['small', 'medium']), // this is enum incase we need to add more sizes | |||
| size: PropTypes.oneOf(Object.values(SIZES)), | |||
There was a problem hiding this comment.
Thoughts on this approach? The array of values is not immediately apparent without jumping to the variable definition, though I like that using the variable means that there is only 1 place where the string value needs to be updated. In docs mode, the array of values looks great.
src/components/Button.js
Outdated
| @@ -233,6 +251,7 @@ const ButtonWrapper = styled.button` | |||
| &:focus { | |||
| box-shadow: ${color.secondary} 0 0 0 3em inset, | |||
| ${rgba(color.secondary, 0.4)} 0 1px 9px 2px; | |||
| color: ${color.lightest}; | |||
There was a problem hiding this comment.
To prevent this in the focus state:
Now it looks like this:
| }) | ||
| expect(ExampleComponent).toBeTruthy(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Prettier updates...
src/components/Button.js
Outdated
| @@ -421,6 +443,7 @@ export function Button({ | |||
| } | |||
|
|
|||
| Button.propTypes = { | |||
| active: PropTypes.bool, | |||
There was a problem hiding this comment.
I tend to prefer using qualifiers for booleans, like isActive or isUnclickable because I think the cognitive load is lower. It makes it obvious that the value will be true or false without having to look up the docs. That said, I am flexible and think it should be agreed upon as a standard so I didn't update anything -- just calling it out to get the conversation started. Thoughts on the approach for future work?
|
@domyen Button updates! Left you a bunch of comments here to get some conversations started. Can you help me out w/ the purpose of the |
|
@kylesuss Just tried |
| containsIcon={containsIcon} | ||
| {...props} | ||
| > | ||
| <ButtonLink isLoading={isLoading} disabled={isDisabled} {...props}> |
There was a problem hiding this comment.
disabled is always a tricky one w/ regard to the is prefixing -- since the default HTML property is just disabled, I typically pass that to the actual button element so that it can do things naturally, but require the prefix on the React component that wraps it.
|
🚀 PR was released in v0.0.13 🚀 |
appearances -- you can no longer combine multiple string appearances.csscalls from within thestyledblocksactivestate. I do not really understand the point of this state though and I would like to add a comment for it for docs mode, so I would like to understand the use caseactivestate--
Over time, I would like to slim up the styles in this component as I think they are a bit hard to follow. It can be hard to tell where certain styles are coming from given there are some duplications in various prop checks and properties. Also, given the styles take up ~300 or so lines, I think there is room to split this out in a way that is a bit easier to track down. It feels like an unnecessary optimization now though, so I believe this would be better served for future work/once it actually becomes a problem.