Sync Label component with PVC's Label component

[mperrotti]

Instead of introducing a breaking change to the existing Label component, I'm proposing that we take the same path we did with ActionList, Button, and others: introduce a "draft" component.

Screenshots

Merge checklist

• Added/updated tests
• Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

Reference

Issue: https://github.com/github/primer/issues/568
PVC's Label component: https://primer.style/view-components/components/label

[changeset-bot]

🦋 Changeset detected

Latest commit: 08b3e11

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[mperrotti]

We can remove these once #1785 is merged

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	61.25 KB (0%)
dist/browser.umd.js	61.63 KB (0%)

[colebemis]

Looks like this prop is called scheme in the code.

Also, I'm curious if we should call this prop variant to be consistent with the Button API: https://primer.style/react/drafts/Button2#button

[mperrotti]

Yup, this is just a mistake. Good catch!

Agreed on changing to variant too. I was just copying the PVC API.

[colebemis]

I wonder if we can use the variants utility from styled-system to clean up all the get() calls here.

[colebemis]

Maybe something like this:

import styled from 'styled-components'
import {sx, SxProp} from './sx'
import { variant, SystemStyleObject } from 'styled-system'

const variants = {
  default: {
    borderColor: 'border.default'
  },
  primary: {
    borderColor: 'fg.default'
  },
  ...
} as const

const sizes = {
  small: {
    height: '20px',
    padding: '7px',
  },
  large: ...
} as const

export type LabelProps = {
  variant: keyof typeof variants
  size: keyof typeof sizes
} & SxProp

const Label = styled.span<LabelProps>`
  ...styles...
  ${variant({ variants })};
  ${variant({ prop: 'size', variants: sizes })};
  ${sx};
`

Label.defaultProps = {
  variant: 'default',
  size: 'small',
}

[mperrotti]

Ah - yea, this feels cleaner. I'll give it a shot 👍

[colebemis]

It seems like this is equivalent to:

const Label: React.FC<Props> = props => <LabelContainer {...props} />

Is there a reason we need to have this proxy Label component? What if we exported the styled component directly?

[mperrotti]

This is leftover from a more complex implementation where we handled a leadingVisual. We can just use the styled component directly.

Good catch.

[rezrah]

Should we use relative units like PVC too? 2em

[mperrotti]

The border-radius isn't relative to anything - it's just a really high number to make it perfectly round.

border-width is also not relative to anything, it's just 1px.

[rezrah]

✅

Was the package-lock change intentional?

[mperrotti]

@rezrah - the package-lock change was not intentional. I'll just pull in whatever is on main