Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync Label component with PVC's Label component #1797

Merged
merged 33 commits into from
Jan 25, 2022
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
5c9018a
introduces new label component
mperrotti Nov 3, 2021
6a66da2
updates storybook stories and adds React docs for Label components
mperrotti Jan 12, 2022
fae2072
adds tests
mperrotti Jan 12, 2022
73e2911
uses hacky fix for rendering the correct Label component in the docs
mperrotti Jan 12, 2022
7c5bcc0
reduces scope of new label components
mperrotti Jan 13, 2022
96aa767
rm StateLabel2
mperrotti Jan 13, 2022
e94b1ec
fix React docs
mperrotti Jan 13, 2022
2e9587b
syncs Label styling with PVC/PCSS
mperrotti Jan 13, 2022
4754dd1
adds a changeset
mperrotti Jan 14, 2022
0863146
renames 'scheme' prop to 'variant'
mperrotti Jan 14, 2022
7bfc14b
Update src/Label2.tsx
mperrotti Jan 14, 2022
8c903c5
Update src/Label2.tsx
mperrotti Jan 14, 2022
cbf7dc6
Merge branch 'main' into mp/label-sync-with-pvc
mperrotti Jan 14, 2022
cb8469e
Merge github.com:primer/react into mp/label-sync-with-pvc
mperrotti Jan 14, 2022
1974be8
addresses more PR feedback
mperrotti Jan 14, 2022
9ad5680
Merge branch 'mp/label-sync-with-pvc' of github.com:primer/react into…
mperrotti Jan 14, 2022
ebb8756
Update docs/content/drafts/Label2.mdx
mperrotti Jan 14, 2022
106a854
Update docs/content/drafts/Label2.mdx
mperrotti Jan 21, 2022
9ff1905
Update src/Label2.tsx
mperrotti Jan 21, 2022
590c7a2
Update src/Label2.tsx
mperrotti Jan 21, 2022
f494349
does not export Label as the default export of Label2.tsx
mperrotti Jan 21, 2022
0e6490b
Merge branch 'mp/label-sync-with-pvc' of github.com:primer/react into…
mperrotti Jan 21, 2022
f015e1f
Update docs/content/drafts/Label2.mdx
mperrotti Jan 21, 2022
a9af0ba
Merge branch 'main' into mp/label-sync-with-pvc
mperrotti Jan 21, 2022
83ddc6d
fix imports
mperrotti Jan 21, 2022
72084d4
Merge branch 'mp/label-sync-with-pvc' of github.com:primer/react into…
mperrotti Jan 21, 2022
7b69c5f
Update src/Label2.tsx
mperrotti Jan 24, 2022
8fe7f81
Update docs/content/drafts/Label2.mdx
mperrotti Jan 24, 2022
6286cf0
Update docs/content/drafts/Label2.mdx
mperrotti Jan 24, 2022
9510286
addresses more PR feedback
mperrotti Jan 24, 2022
659e6c8
Merge branch 'main' into mp/label-sync-with-pvc
mperrotti Jan 25, 2022
4f612a5
revert package-lock to main
mperrotti Jan 25, 2022
08b3e11
Merge branch 'mp/label-sync-with-pvc' of github.com:primer/react into…
mperrotti Jan 25, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/stale-badgers-search.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

Introduces a draft for component to replace the existing Label component
93 changes: 93 additions & 0 deletions docs/content/drafts/Label2.mdx
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
---
title: Label
mperrotti marked this conversation as resolved.
Show resolved Hide resolved
componentId: label2
status: Alpha
source: https://github.com/primer/react/tree/main/src/Label2
storybook: '/react/storybook?path=/story/label-label2--label'
mperrotti marked this conversation as resolved.
Show resolved Hide resolved
description: Use Label components to add contextual metadata to a design.
---

## Examples

### Basic

```javascript live noinline
// import {Label} from '@primer/react/drafts'
const {Label} = drafts // ignore docs silliness; import like that ↑

render(<Label>Default</Label>)
```

### Appearances
mperrotti marked this conversation as resolved.
Show resolved Hide resolved

```javascript live noinline
// import {Label} from '@primer/react/drafts'
const {Label} = drafts // ignore docs silliness; import like that ↑
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove these once #1785 is merged

render(
<>
<Label>Default</Label>
<Label scheme="primary">Primary</Label>
<Label scheme="secondary">Secondary</Label>
<Label scheme="accent">Info</Label>
<Label scheme="success">Success</Label>
<Label scheme="attention">Warning</Label>
<Label scheme="severe">Severe</Label>
<Label scheme="danger">Danger</Label>
<Label scheme="done">Done</Label>
<Label scheme="sponsors">Sponsors</Label>
</>
)
```

### Sizes

```javascript live noinline
// import {Label} from '@primer/react/drafts'
const {Label} = drafts // ignore docs silliness; import like that ↑
render(
<>
<Label>Small (default)</Label>
<Label size="large">Large</Label>
</>
)
```

## Props

### Label

<PropsTable>
<PropsTableRow
name="appearance"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mperrotti mperrotti Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this is just a mistake. Good catch!

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

type="'default' | 'primary' | 'secondary' | 'accent' | 'success' | 'attention' | 'severe' | 'danger' | 'done' | 'sponsors'"
mperrotti marked this conversation as resolved.
Show resolved Hide resolved
defaultValue="'default'"
description="The color of the label"
/>
<PropsTableRow
name="size"
type="'small' | 'large'"
defaultValue="'small'"
description="How large the label is rendered"
/>
</PropsTable>

## Status

<ComponentChecklist
items={{
propsDocumented: true,
noUnnecessaryDeps: true,
adaptsToThemes: true,
adaptsToScreenSizes: true,
fullTestCoverage: true,
usedInProduction: false,
usageExamplesDocumented: true,
hasStorybookStories: true,
designReviewed: false,
a11yReviewed: false,
stableApi: false,
addressedApiFeedback: false,
hasDesignGuidelines: false,
hasFigmaComponent: false
}}
/>
141 changes: 29 additions & 112 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

114 changes: 114 additions & 0 deletions src/Label2.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import React from 'react'
import styled, {css} from 'styled-components'
import {get} from './constants'

interface Props {
/** The color of the label */
scheme?: LabelColorOptions
/** How large the label is rendered */
size?: LabelSizeKeys
}
interface LabelColorConfig {
borderColor: (props: Props) => React.CSSProperties['color']
textColor?: (props: Props) => React.CSSProperties['color']
}
export type LabelColorOptions =
| 'default'
| 'primary'
| 'secondary'
| 'accent'
| 'success'
| 'attention'
| 'severe'
| 'danger'
| 'done'
| 'sponsors'

type LabelSizeKeys = 'small' | 'large'

export const labelColorMap: Record<LabelColorOptions, LabelColorConfig> = {
default: {
borderColor: get('colors.border.default')
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

primary: {
borderColor: get('colors.fg.default')
},
secondary: {
borderColor: get('colors.border.muted'),
textColor: get('colors.fg.muted')
},
accent: {
borderColor: get('colors.accent.emphasis'),
textColor: get('colors.accent.fg')
},
success: {
borderColor: get('colors.success.emphasis'),
textColor: get('colors.success.fg')
},
attention: {
borderColor: get('colors.attention.emphasis'),
textColor: get('colors.attention.fg')
},
severe: {
borderColor: get('colors.severe.emphasis'),
textColor: get('colors.severe.fg')
},
danger: {
borderColor: get('colors.danger.emphasis'),
textColor: get('colors.danger.fg')
},
done: {
borderColor: get('colors.done.fg'),
textColor: get('colors.done.emphasis')
},
sponsors: {
borderColor: get('colors.sponsors.fg'),
textColor: get('colors.sponsors.emphasis')
}
}

const badgeBoxStyle: Record<LabelSizeKeys, Record<'height' | 'padding', number>> = {
small: {
height: 20,
padding: 7 // hard-coded to align with Primer ViewCompnents and Primer CSS
},
large: {
height: 24,
padding: 10 // hard-coded to align with Primer ViewCompnents and Primer CSS
}
}

const LabelContainer = styled.span<Props>`
align-items: center;
border-width: 1px;
border-radius: 999px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use relative units like PVC too? 2em

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

border-style: solid;
display: inline-flex;
font-weight: ${get('fontWeights.bold')};
font-size: ${get('fontSizes.0')};
line-height: 1;
white-space: nowrap;

${({scheme = 'default', size = 'small'}) => {
return css`
background-color: transparent;
border-color: ${labelColorMap[scheme].borderColor};
color: ${labelColorMap[scheme].textColor};
height: ${badgeBoxStyle[size].height}px;
padding: 0 ${badgeBoxStyle[size].padding}px;
`
}}
mperrotti marked this conversation as resolved.
Show resolved Hide resolved
`

const Label: React.FC<Props> = ({children, size, ...other}) => (
<LabelContainer size={size} {...other}>
{children}
</LabelContainer>
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Good catch.


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

export default Label
Loading