Merge Status Component #62
Conversation
| color ? `State--${color}` : null | ||
| )}> | ||
| {icon ? <span className='mr-1'>{icon}</span> : null} | ||
| color && color !== 'yellow' ? `State--${color}` : null, |
There was a problem hiding this comment.
This hack doesn't feel great, will be much cleaner if we can add the yellow state to primer/primer
src/StateLabel.js
Outdated
| const color = scheme || stateColorMap[state] | ||
| let styles = '' | ||
| if (color === 'yellow') { | ||
| styles = { backgroundColor: colors.yellow[7]} |
There was a problem hiding this comment.
Nit: inconsistent whitespace inside the curlies. 
examples/index.js
Outdated
| <Block> | ||
| <Heading tag="h2" mb={1}>Merge Box Status</Heading> | ||
| <span className='mr-2'> | ||
| <StateLabel scheme="green" small icon={<Octicon name='git-merge'/>}/> |
There was a problem hiding this comment.
These should be MergeStatus instead of StateLabel, right?
There was a problem hiding this comment.
Yeah I need to remove that one from the examples! For clarification, this PR isn't ready for a code review yet :) Just wanted to get some input on the items in the PR description and thought it would be easier to do that here than dumping in Slack!
|
|
||
| export default function MergeStatus({ state }) { | ||
| return ( | ||
| <StateLabel scheme={stateColorMap[state]} small icon={<Octicon name='git-merge'/>} /> |
There was a problem hiding this comment.
Would it be better to initialize the Octicon once and pass it by reference here or not? E.g.
const mergeOcticon = <Octicon name='git-merge' />
// ...
<StateLabel scheme={...} small icon={mergeOcticon} />
| if (icon && children) { | ||
| return <span className='mr-1'>{icon}</span> | ||
| } else if (icon) { | ||
| return <span className='d-flex m-1'>{icon}</span> |
There was a problem hiding this comment.
Why the d-flex here if there's only one child?
There was a problem hiding this comment.
The d-flex here vertically centers the icon in the component by adding some extra vertical space
Before:
After:
There was a problem hiding this comment.
Oh, cool! I never would've thought to do it that way.
examples/index.js
Outdated
| </Example> | ||
| <Example name="MergeStatus"> | ||
| <span className='mr-2'> | ||
| <MergeStatus state="pending"/> |
There was a problem hiding this comment.
😭 old habits die hard! I have to get that linter running ASAP
Opening this PR mostly to facilitate discussion!
Merge Status icon:
Also see all the variations here: https://user-images.githubusercontent.com/54012/40739554-5c4a6ce4-63fb-11e8-8ad7-ff2e5eff06ba.png
It looks like this component can be created using our StateLabel component that we've already got with some minor tweaks to accommodate the case of only having an icon and no text in a label.
Some issues:
the State primer/primer css doesn't include a yellow state for yellow. So I had to put in a hack around that to get StateLabel to play nicely with the yellow color we need. I think we should add a
State--yellowto primer/primer to fix this, since that yellow color seems to be used in a few places relating to state (pending dots on checks, merge status icon, etc)the StateLabel component
stateprops match up nicely with issues, but don't match up as nicely with the Merge Status label, so I've create a MergeStatus component to map between MergeStatus states and the schemes in StateLabel. I wonder if a better approach would be to make StateLabel a little more generic, since right now it seems a bit coupled with Issues, but curious what y'all think!