Refactor StateLabel 🚀 #311

emplums · 2018-10-11T00:21:21Z

This PR refactors the StateLabel component using Emotion + gets rid of remaining Primer CSS classes & takes a stab at rethinking the prop API 🙀 PLUS addresses: #232 and #257 and #191.

🚨 BREAKING CHANGES! 🚨

Removes the state prop. I felt like having a state prop was a bit overkill on this component, and the API would be much simpler if you allowed users to pass any Icon + any scheme instead of trying to map the state to an icon + background color.
Changes the behavior of the icon prop -> instead of passing a full Octiocon wrapper, just pass the icon you'd like to use. Example: icon={Check}

Notes:
To make it even more flexible we could even remove the scheme prop and have users set that via bg, but since the State component in Primer CSS right now sets some pretty specific colors, I thought it would be nice to keep the scheme prop.

See additional comments in code.

Closes #232
Closes #257
Closes #191

Merge checklist

vercel · 2018-10-11T00:21:25Z

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

broccolini · 2018-10-11T14:02:07Z

src/StateLabel.js

+  display: inline-flex;
+  align-items: center;
+  padding: ${props => (props.small ? `0.125em ${theme.space[1]}px` : `${theme.space[1]}px ${theme.space[2]}px`)};
+  font-weight: 600;


If we have fontWeight defined in our theme can we reference the theme value here?

We could, but it's not defined yet - I can add it in, what font weights should we use?

Agreed: we should add fontWeight to the list of system props and set defaultProps.fontWeight = 'bold' rather than hard-coding this.

I don't think we want fontWeight to be a prop on this component, but yeah I agree we should add font weight to our theme! We can do that as a follow up, and replace any other instances at that time.

Ah, so maybe: font-weight: ${themeGet('fontWeights.bold')}?

broccolini · 2018-10-11T14:33:23Z

Thanks for working on this @emplums the current StateLabel api is definitely confusing and needs work. After looking at this it's raising some questions some questions for me (apologies if I'm contradicting my past self):

Isn't this just a variation of Label? It's just bigger and works nicely with an icon.
GitHub needs labels for specific use cases—open, closed, merged, gray?!—with and without icons. If we have StateLabels in here we should make them easy to use (we might also argue they live in dotcom).

Perhaps we should make a more flexible label that takes any background color, has small, medium, large, xl sizes with an option for pixel value, and optionally can have a drop-shadow (in dotcom Labels sometimes have drop shadows, sometimes don't). Then make a less flexible but specific StateLabel that accounts for the states we need, with the props for size, and with or without icon.

Example StateLabel

<StateLabel scheme="open" withIcon small />

Schemes would be open, closed, merged, and default props when no scheme provided would be gray.
Icon prop probably needs more thought, not sure if should be boolean, and not sure what the default should be.

Example Label

<Label bg="blue.5" color="white" icon={Heart} large  dropShadow />

Default props should probably be gray with white text.

emplums · 2018-10-11T15:49:17Z

@broccolini I'm cool with this! I'll refactor to see how it feels

emplums · 2018-10-11T17:04:17Z

Ok @broccolini I've got this a little closer - the StateLabel component should have the API you've mentioned, except I didn't add the withIcon prop - I figured if a user wanted a StateLabel without an icon, they'd just use Label?

I'm still working through Label - it's got most of the API you've mentioned, but I'm still thinking through how to handle the size props. Should the sizes correspond to the amount of padding that we add to the element? If so, do you have any suggestions on what the levels should be? The current label (which I'm guessing will correspond to small?) has 3px 4px padding. How does this feel for the other levels?

small: 3px 4px (current Primer CSS styling of .Label)
medium: 4px 6px
large: 6px 8px
x-large: 8px 10px

broccolini · 2018-10-11T18:41:22Z

StateLabel

I didn't add the withIcon prop - I figured if a user wanted a StateLabel without an icon, they'd just use Label?

Yeah I thought that maybe we used the label a lot without icons but I can't find any instances 🤔. Can always add later if needed.

I did just realize we need to account for issues as well as pull requests though. They have different Icons but the same colors. Schemes would be somethings like issueOpen, pullOpen, issueClosed, pullClosed, pullMerged - open to other suggestions.

Labels

To get this right we'll need to do an audit in dotcom, however I think you could ship something that heads in the right direction. Below are some examples from dotcom based off this issue https://github.com/github/design-systems/issues/444 in our design systems repo.

Issue sidebar labels:

    height: 20px;
    padding: 0.15em 4px;
    font-size: 12px;
    line-height: 15px;
    border-radius: 2px;

Large/xl labels in issues:

    padding: 0 8px;
    font-size: 16px;
    line-height: 2;
    border-radius: 3px;

State labels:

    padding: 4px 8px;
    font-size: 14px;
    line-height: 20px;
    border-radius: 3px;

Based on this quick audit, these sizes might work and attempts to cover some of the common sizes. But please adjust if doesn't work in practice. We might also consider using ems entirely instead of pixels.

small = current default label size
medium = current small state label size padding 0.125em 8px; font-size: 12px
large = current default state label size padding: 4px 8px; font-size: 14px
xl = similar to large issue label size padding: 4px 8px; font-size: 16px

Only question I have is which one we make the default. It feels like the current default label size, small, will be the most commonly used, but feels odd to have medium, large, and xl size props and not small.

emplums · 2018-10-11T20:50:16Z

@broccolini what if we did:
small = padding 0.125em 8px; font-size: 12px
medium = our current default size -> padding: 2px 3px; font-size: 12px
large = padding: 4px 8px; font-size: 14px
xl = padding: 4px 8px; font-size: 16px

and make medium the default?? I think it makes more sense to call padding 0.125em 8px; font-size: 12px small, since it's smaller than the default of padding: 2px 3px; font-size: 12px. and then that way the default being medium makes a little more sense, since you can make it smaller using the small size or larger using the large size

broccolini · 2018-10-11T22:21:22Z

Sorry the current small state label is 4px not 8px! padding 0.125em 4px; font-size: 12px

I'm fine going with the above solution. My only concern is that 95% of the time when we use labels they will need to add the small prop, but I'm not actually sure that's true and maybe we should just go bigger anyway!

emplums · 2018-10-11T22:36:02Z

@broccolini ahh ok that makes more sense 😂 It looked super weird with 8px! I'm not sure if you noticed but I made the current sizing (that's default in .Label) "medium" and the one that is a bit smaller, "small".

I also added in the different states for issues + PRs on state labels, so the schemes now are: issueOpened, issueClosed, pullOpened, pullClosed and pullMerged!

emplums · 2018-10-11T22:39:05Z

src/Label.js

  }
-}
+  ${color} ${props => (props.dropshadow ? 'box-shadow: inset 0 -1px 0 rgba(27, 31, 35, 0.12)' : '')};


eslint is making me put these on the same line, though it hurts my eyezzzzzzzzz

emplums · 2018-10-11T22:40:18Z

src/Label.js

  }
-}
+  ${color} //eslint-disable-line


I had to put this here in order to get eslint to shut up about this being on it's own line 😭

That's crazy. What if you put a semicolon at the end?

shawnbot · 2018-10-11T22:42:22Z

src/StateLabel.js

-    color && color !== 'yellow' ? `State--${color}` : null
-  )
+const getOcticon = (scheme, small) =>
+  small ? <Octicon mr={1} width="1em" icon={octiconMap[scheme]} /> : <Octicon mr={1} icon={octiconMap[scheme]} />


This smells funky to me since all it's actually varying is the width prop. Could we inline it back in the render function like so?

<Octicon icon={octiconMap[scheme]} mr={1} width={small ? '1em' : null} />

No, because setting the width to null here overrides the width set in the SVG and makes it the wrong size. I tried that first!

Oof. Maybe something like this? 😬

const octiconProps = small ? {width: '1em'} : {} // ... <Octicon mr={1} icon={octiconMap[scheme]} {...octiconProps} />

shawnbot

Sorry, I got a bit in the weeds with this review. I left some comments about some specific bits, but they're not blockers by any stretch. Let's ship this and refactor again, hopefully when we have a nicer way of specifying variants in place.

Emily added 8 commits October 10, 2018 16:16

refactor StateLabel using emotion

6f97674

update docs

7c463cf

refactor Octicon handling & add small prop

0e68ae2

update tests

5141166

update example

372a2d1

refactor Octicon + use theme values

2d8cd2b

use theme color

97a3f23

replace border radius with theme value

13006a2

fix lint errors

71db707

emplums requested review from shawnbot, jonrohan and broccolini October 11, 2018 00:25

shawnbot mentioned this pull request Oct 11, 2018

Q3 Pink Robots Tracking PR #310

Merged

12 tasks

broccolini reviewed Oct 11, 2018

View reviewed changes

broccolini added the enhancement New feature or request label Oct 11, 2018

Emily added 7 commits October 11, 2018 08:57

refactor StateLabel

3b3b7cd

update readme

0b46f6f

update props

eb3964b

lint

639fae9

update tests

b9d8010

lint again

b10cdaa

add dropshadow prop

efa0421

emplums changed the base branch from master to q3-pink-robots October 11, 2018 17:08

add size prop

a489821

Emily added 5 commits October 11, 2018 15:27

clean up Label CSS

035f725

update StateLabel docs

8944d90

change schemes

ca89f3d

update snapshots

6dd5c26

lint

cebe780

emplums commented Oct 11, 2018

View reviewed changes

use eslint disable line

49d60c7

emplums commented Oct 11, 2018

View reviewed changes

tweak ordering of css

16597f5

shawnbot reviewed Oct 11, 2018

View reviewed changes

emplums mentioned this pull request Oct 11, 2018

add fontWeight to theme #318

Closed

1 task

shawnbot approved these changes Oct 11, 2018

View reviewed changes

Emily added 4 commits October 12, 2018 10:57

tweak Octicon props

e400c6e

update snapshot

aac775e

remove eslint disable

721c420

add COMMON props to Label

bc20ec8

emplums merged commit 3d15708 into q3-pink-robots Oct 12, 2018

emplums deleted the state-label-tweaks branch October 12, 2018 18:10

Refactor StateLabel 🚀 #311

Refactor StateLabel 🚀 #311

Uh oh!

Conversation

emplums commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge checklist

Uh oh!

vercel bot commented Oct 11, 2018

Uh oh!

broccolini Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

broccolini commented Oct 11, 2018

Example StateLabel

Example Label

Uh oh!

emplums commented Oct 11, 2018

Uh oh!

emplums commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

broccolini commented Oct 11, 2018

StateLabel

Labels

Uh oh!

emplums commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

broccolini commented Oct 11, 2018

Uh oh!

emplums commented Oct 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shawnbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emplums commented Oct 11, 2018 •

edited

Loading

broccolini Oct 11, 2018 •

edited

Loading

emplums commented Oct 11, 2018 •

edited

Loading

emplums commented Oct 11, 2018 •

edited

Loading