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

Update token components' APIs #1998

Merged
merged 8 commits into from
Mar 29, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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/six-trains-guess.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

Updates the API for token components to align with our size-naming ADR, avatar guidelines, and icon guidelines
4 changes: 2 additions & 2 deletions docs/content/TextInputWithTokens.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -419,8 +419,8 @@ render(<WithIconAndLoadingIndicator />)
/>
<PropsTableRow
name="size"
type="'small' | 'medium' | 'large' | 'extralarge'"
defaultValue="extralarge"
type="'small' | 'medium' | 'large' | 'xlarge'"
defaultValue="xlarge"
description="The size of the tokens and text input"
/>
<PropsTableRow
Expand Down
85 changes: 36 additions & 49 deletions docs/content/Token.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ All types of tokens may have the same interactive qualities as links or buttons.
<Text fontSize={0} display="block" color="fg.subtle">
With `leadingVisual` passed
</Text>
<Token text="Default Token" leadingVisual={() => <span>✨</span>} />
<Token text="Default Token" leadingVisual={GitBranchIcon} />
</div>

<div>
Expand All @@ -81,39 +81,37 @@ All types of tokens may have the same interactive qualities as links or buttons.
gap: 2
}}
>
<Token
size="small"
text="'small' Token"
onRemove={() => {
console.log('remove me')
}}
/>
<Token
size="medium"
text="'medium' Token"
onRemove={() => {
console.log('remove me')
}}
/>
<Token
size="large"
text="'large' Token (default)"
onRemove={() => {
console.log('remove me')
}}
/>
<Token
size="extralarge"
text="'extralarge' Token"
onRemove={() => {
console.log('remove me')
}}
/>
<Token size="small" text="'small' Token" />
<Token size="medium" text="'medium' Token" />
<Token size="large" text="'large' Token (default)" />
<Token size="xlarge" text="'xlarge' Token" />
</Box>
</div>
</Box>
```

### With leadingVisual

<Note variant="warning">
A <InlineCode>leadingVisual</InlineCode> should not be used with the <InlineCode>small</InlineCode> size option if
you're rendering an icon from Octicons
</Note>

```jsx live
<Box
display="flex"
sx={{
alignItems: 'start',
flexDirection: 'column',
gap: 6
}}
>
<Token size="medium" text="'medium' Token" leadingVisual={GitBranchIcon} />
<Token size="large" text="'large' Token (default)" leadingVisual={GitBranchIcon} />
<Token size="xlarge" text="'xlarge' Token" leadingVisual={GitBranchIcon} />
</Box>
```

### Interactive tokens

If any token is interactive (it is a link or a button), it will become focusable, and react to being hovered or focused on.
Expand Down Expand Up @@ -267,8 +265,8 @@ Tokens that represent Issue labels should use the `IssueLabelToken` component.
/>
<IssueLabelToken
fillColor="#0366d6"
size="extralarge"
text="'extralarge' Token"
size="xlarge"
text="'xlarge' Token"
onRemove={() => {
console.log('remove me')
}}
Expand All @@ -282,6 +280,11 @@ Tokens that represent Issue labels should use the `IssueLabelToken` component.

Tokens that represent GitHub users should use the `AvatarToken` component.

<Note variant="warning">
This component should not be used with the <InlineCode>small</InlineCode> or <InlineCode>medium</InlineCode> size
option
</Note>

```jsx live
<Box
display="flex"
Expand Down Expand Up @@ -329,22 +332,6 @@ Tokens that represent GitHub users should use the `AvatarToken` component.
gap: 2
}}
>
<AvatarToken
avatarSrc="https://avatars.githubusercontent.com/mperrotti"
size="small"
text="'small' Token"
onRemove={() => {
console.log('remove me')
}}
/>
<AvatarToken
avatarSrc="https://avatars.githubusercontent.com/mperrotti"
size="medium"
text="'medium' Token"
onRemove={() => {
console.log('remove me')
}}
/>
<AvatarToken
avatarSrc="https://avatars.githubusercontent.com/mperrotti"
size="large"
Expand All @@ -355,8 +342,8 @@ Tokens that represent GitHub users should use the `AvatarToken` component.
/>
<AvatarToken
avatarSrc="https://avatars.githubusercontent.com/mperrotti"
size="extralarge"
text="'extralarge' Token"
size="xlarge"
text="'xlarge' Token"
onRemove={() => {
console.log('remove me')
}}
Expand Down
8 changes: 5 additions & 3 deletions src/TextInputWithTokens.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ const overflowCountFontSizeMap: Record<TokenSizeKeys, number> = {
small: 0,
medium: 1,
large: 1,
extralarge: 2
extralarge: 2,
xlarge: 2 // will eventually replace "extralarge" per this ADR: https://github.com/github/primer/blob/main/adrs/2022-02-09-size-naming-guidelines.md
}

// using forwardRef is important so that other components (ex. Autocomplete) can use the ref
Expand Down Expand Up @@ -239,7 +240,8 @@ function TextInputWithTokensInnerComponent<TokenComponentType extends AnyReactCo
small: 'small',
medium: 'small',
large: 'medium',
extralarge: 'medium'
extralarge: 'medium',
xlarge: 'medium' // will eventually replace "extralarge" per this ADR: https://github.com/github/primer/blob/main/adrs/2022-02-09-size-naming-guidelines.md
Copy link
Contributor

@rezrah rezrah Mar 28, 2022

Choose a reason for hiding this comment

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

I think this adr is on a private repo, can we remove the url?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this link is helpful for maintainers. I think it's okay that's internal-only

}
const showLeadingLoadingIndicator =
loading && (loaderPosition === 'leading' || Boolean(LeadingVisual && loaderPosition !== 'trailing'))
Expand Down Expand Up @@ -370,7 +372,7 @@ const TextInputWithTokens = React.forwardRef(TextInputWithTokensInnerComponent)

TextInputWithTokens.defaultProps = {
tokenComponent: Token,
size: 'extralarge',
size: 'xlarge',
hideTokenRemoveButtons: false,
preventTokenWrapping: false,
loaderPosition: 'auto'
Expand Down
1 change: 1 addition & 0 deletions src/Token/AvatarToken.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {TokenBaseProps, defaultTokenSize, tokenSizes, TokenSizeKeys} from './Tok
import Token from './Token'
import {Avatar} from '..'

// TODO: update props to only accept 'large' and 'xlarge' on the next breaking change
export interface AvatarTokenProps extends TokenBaseProps {
avatarSrc: string
}
Expand Down
4 changes: 0 additions & 4 deletions src/Token/IssueLabelToken.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@ export interface IssueLabelTokenProps extends TokenBaseProps {
* The color that corresponds to the label
*/
fillColor?: string
/**
* Whether the remove button should be rendered in the token
*/
hideRemoveButton?: boolean
}

const tokenBorderWidthPx = 1
Expand Down
5 changes: 1 addition & 4 deletions src/Token/Token.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,6 @@ export interface TokenProps extends TokenBaseProps {
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
leadingVisual?: React.ComponentType<any>
/**
* Whether the remove button should be rendered in the token
*/
hideRemoveButton?: boolean
}

const tokenBorderWidthPx = 1
Expand Down Expand Up @@ -52,6 +48,7 @@ const LeadingVisualContainer = styled('span')<Pick<TokenBaseProps, 'size'>>`
switch (props.size) {
case 'large':
case 'extralarge':
case 'xlarge':
return css`
margin-right: ${get('space.2')};
`
Expand Down
34 changes: 23 additions & 11 deletions src/Token/TokenBase.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,18 @@ import {variant} from 'styled-system'
import {get} from '../constants'
import sx, {SxProp} from '../sx'

export type TokenSizeKeys = 'small' | 'medium' | 'large' | 'extralarge'
// TODO: remove invalid "extralarge" size name in next breaking change
// ADR: https://github.com/github/primer/blob/main/adrs/2022-02-09-size-naming-guidelines.md
export type TokenSizeKeys = 'small' | 'medium' | 'large' | 'extralarge' | 'xlarge'
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding a @deprecated flag to the extralarge value here?

We'd need to bump to minor instead, but feels like we can give early notice that we'll remove it more formally later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to add the @deprecated flag to specific values?

Copy link
Contributor

@rezrah rezrah Mar 29, 2022

Choose a reason for hiding this comment

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

It's not supported end-to-end yet, but you can get it working for anyone browsing the codebase by hoisting it into a separate type and then reapplying it in the union or as an intersection. Maybe someone will find it useful 🤷

E.g. something like:

Suggested change
export type TokenSizeKeys = 'small' | 'medium' | 'large' | 'extralarge' | 'xlarge'
/** @deprecated to be removed in v36.. probably **/
type ExtraLarge = 'extralarge'
export type TokenSizeKeys =
| 'small'
| 'medium'
| 'large'
| 'xlarge'
| ExtraLarge

will show this..
Screenshot 2022-03-29 at 14 29 53


const xlargeSize = '32px'

export const tokenSizes: Record<TokenSizeKeys, string> = {
small: '16px',
medium: '20px',
large: '24px',
extralarge: '32px'
extralarge: xlargeSize,
xlarge: xlargeSize
}

export const defaultTokenSize: TokenSizeKeys = 'medium'
Expand All @@ -22,6 +27,10 @@ export interface TokenBaseProps
* The function that gets called when a user clicks the remove button, or keys "Backspace" or "Delete" when focused on the token
*/
onRemove?: () => void
/**
* Whether the remove button should be rendered in the token
*/
hideRemoveButton?: boolean
/**
* Whether the token is selected
*/
Expand All @@ -43,6 +52,16 @@ export interface TokenBaseProps
export const isTokenInteractive = ({as = 'span', onClick, onFocus, tabIndex = -1}: TokenBaseProps) =>
Boolean(onFocus || onClick || tabIndex > -1 || ['a', 'button'].includes(as))

const xlargeVariantStyles = {
fontSize: 1,
height: tokenSizes.xlarge,
lineHeight: tokenSizes.xlarge,
paddingLeft: 3,
paddingRight: 3,
paddingTop: 0,
paddingBottom: 0
}

const variants = variant<
{
fontSize: number
Expand Down Expand Up @@ -87,15 +106,8 @@ const variants = variant<
paddingTop: 0,
paddingBottom: 0
},
extralarge: {
fontSize: 1,
height: tokenSizes.extralarge,
lineHeight: tokenSizes.extralarge,
paddingLeft: 3,
paddingRight: 3,
paddingTop: 0,
paddingBottom: 0
}
extralarge: xlargeVariantStyles,
xlarge: xlargeVariantStyles
}
})

Expand Down
6 changes: 6 additions & 0 deletions src/Token/_RemoveTokenButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ const variants = variant<{height: string; width: string}, TokenSizeKeys>({
extralarge: {
height: tokenSizes.extralarge,
width: tokenSizes.extralarge
},
// xlarge will eventually replace "extralarge" per this ADR: https://github.com/github/primer/blob/main/adrs/2022-02-09-size-naming-guidelines.md
xlarge: {
height: tokenSizes.xlarge,
width: tokenSizes.xlarge
}
}
})
Expand Down Expand Up @@ -58,6 +63,7 @@ const StyledTokenButton = styled.span<TokenButtonProps & SxProp>`
switch (props.size) {
case 'large':
case 'extralarge':
case 'xlarge':
return css`
margin-left: ${get('space.2')};
`
Expand Down
Loading