-
Notifications
You must be signed in to change notification settings - Fork 536
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
Changes from 5 commits
c77e73b
063bbaa
f0f6b39
acaf018
282079b
6329b67
cc4c090
931d73b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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' | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about adding a We'd need to bump to minor instead, but feels like we can give early notice that we'll remove it more formally later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to add the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||||||||||||||||
|
||||||||||||||||||||
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' | ||||||||||||||||||||
|
@@ -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 | ||||||||||||||||||||
*/ | ||||||||||||||||||||
|
@@ -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 | ||||||||||||||||||||
|
@@ -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 | ||||||||||||||||||||
} | ||||||||||||||||||||
}) | ||||||||||||||||||||
|
||||||||||||||||||||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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