Fixes onRemove console error coming from token components (#2087)

* prevents onRemove prop from being passed from token components though to the HTML element * adds changeset
primer · May 20, 2022 · b319b29 · b319b29
1 parent 0530103
commit b319b29
Show file tree

Hide file tree

Showing 5 changed files with 1,734 additions and 1,899 deletions.
diff --git a/.changeset/grumpy-crabs-compete.md b/.changeset/grumpy-crabs-compete.md
@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+Prevents the `onRemove` prop from being passed through to the HTML element from Token, AvatarToken, and IssueToken.
diff --git a/src/Token/Token.tsx b/src/Token/Token.tsx
@@ -1,7 +1,6 @@
 import React, {forwardRef, MouseEventHandler} from 'react'
-import styled, {css} from 'styled-components'
-import {get} from '../constants'
-import sx, {SxProp} from '../sx'
+import {Box} from '..'
+import {merge, SxProp} from '../sx'
 import TokenBase, {defaultTokenSize, isTokenInteractive, TokenBaseProps} from './TokenBase'
 import RemoveTokenButton from './_RemoveTokenButton'
 import TokenTextContainer from './_TokenTextContainer'
@@ -16,53 +15,33 @@ export interface TokenProps extends TokenBaseProps {
 
 const tokenBorderWidthPx = 1
 
-const DefaultTokenStyled = styled(TokenBase)<TokenProps & {isTokenInteractive: boolean} & SxProp>`
-  background-color: ${get('colors.neutral.subtle')};
-  border-color: ${props => (props.isSelected ? get('colors.fg.default') : get('colors.border.subtle'))};
-  border-style: solid;
-  border-width: ${tokenBorderWidthPx}px;
-  color: ${props => (props.isSelected ? get('colors.fg.default') : get('colors.fg.muted'))};
-  max-width: 100%;
-  padding-right: ${props => (!props.hideRemoveButton ? 0 : undefined)};
-  position: relative;
-  ${sx}
-
-  ${props => {
-    if (props.isTokenInteractive) {
-      return css`
-        &:hover {
-          background-color: ${get('colors.neutral.muted')};
-          box-shadow: ${get('colors.shadow.medium')};
-          color: ${get('colors.fg.default')};
-        }
-      `
-    }
-  }}
-`
-
-const LeadingVisualContainer = styled('span')<Pick<TokenBaseProps, 'size'>>`
-  flex-shrink: 0;
-  line-height: 0;
-
-  ${props => {
-    switch (props.size) {
-      case 'large':
-      case 'extralarge':
-      case 'xlarge':
-        return css`
-          margin-right: ${get('space.2')};
-        `
-      default:
-        return css`
-          margin-right: ${get('space.1')};
-        `
-    }
-  }}
-`
+const LeadingVisualContainer: React.FC<Pick<TokenBaseProps, 'size'>> = ({children, size}) => (
+  <Box
+    sx={{
+      flexShrink: 0,
+      lineHeight: 0,
+      marginRight: size && ['large', 'extralarge', 'xlarge'].includes(size) ? 2 : 1
+    }}
+  >
+    {children}
+  </Box>
+)
 
 const Token = forwardRef<HTMLAnchorElement | HTMLButtonElement | HTMLSpanElement, TokenProps & SxProp>(
   (props, forwardedRef) => {
-    const {as, onRemove, id, leadingVisual: LeadingVisual, text, size, hideRemoveButton, href, onClick, ...rest} = props
+    const {
+      as,
+      onRemove,
+      id,
+      leadingVisual: LeadingVisual,
+      text,
+      size,
+      hideRemoveButton,
+      href,
+      onClick,
+      sx: sxProp = {},
+      ...rest
+    } = props
     const hasMultipleActionTargets = isTokenInteractive(props) && Boolean(onRemove) && !hideRemoveButton
     const onRemoveClick: MouseEventHandler = e => {
       e.stopPropagation()
@@ -73,15 +52,35 @@ const Token = forwardRef<HTMLAnchorElement | HTMLButtonElement | HTMLSpanElement
       href,
       onClick
     }
+    const sx = merge(
+      {
+        backgroundColor: 'neutral.subtle',
+        borderColor: props.isSelected ? 'fg.default' : 'border.subtle',
+        borderStyle: 'solid',
+        borderWidth: `${tokenBorderWidthPx}px`,
+        color: props.isSelected ? 'fg.default' : 'fg.muted',
+        maxWidth: '100%',
+        paddingRight: !(hideRemoveButton || !onRemove) ? 0 : undefined,
+        ...(isTokenInteractive(props)
+          ? {
+              '&:hover': {
+                backgroundColor: 'neutral.muted',
+                boxShadow: 'shadow.medium',
+                color: 'fg.default'
+              }
+            }
+          : {})
+      },
+      sxProp as SxProp
+    )
 
     return (
-      <DefaultTokenStyled
+      <TokenBase
         onRemove={onRemove}
-        hideRemoveButton={hideRemoveButton || !onRemove}
         id={id?.toString()}
         text={text}
         size={size}
-        isTokenInteractive={isTokenInteractive(props)}
+        sx={sx}
         {...(!hasMultipleActionTargets ? interactiveTokenProps : {})}
         {...rest}
         ref={forwardedRef}
@@ -109,7 +108,7 @@ const Token = forwardRef<HTMLAnchorElement | HTMLButtonElement | HTMLSpanElement
             }
           />
         ) : null}
-      </DefaultTokenStyled>
+      </TokenBase>
     )
   }
 )

diff --git a/src/Token/TokenBase.tsx b/src/Token/TokenBase.tsx
@@ -1,4 +1,4 @@
-import {KeyboardEvent} from 'react'
+import React, {KeyboardEvent} from 'react'
 import styled from 'styled-components'
 import {variant} from 'styled-system'
 import {get} from '../constants'
@@ -50,7 +50,14 @@ export interface TokenBaseProps
   size?: TokenSizeKeys
 }
 
-export const isTokenInteractive = ({as = 'span', onClick, onFocus, tabIndex = -1}: TokenBaseProps) =>
+type TokenElements = HTMLSpanElement | HTMLButtonElement | HTMLAnchorElement
+
+export const isTokenInteractive = ({
+  as = 'span',
+  onClick,
+  onFocus,
+  tabIndex = -1
+}: Pick<TokenBaseProps, 'as' | 'onClick' | 'onFocus' | 'tabIndex'>) =>
   Boolean(onFocus || onClick || tabIndex > -1 || ['a', 'button'].includes(as))
 
 const xlargeVariantStyles = {
@@ -112,28 +119,40 @@ const variants = variant<
   }
 })
 
-const TokenBase = styled.span.attrs<TokenBaseProps>(({text, onRemove, onKeyDown}) => ({
-  onKeyDown: (event: KeyboardEvent<HTMLSpanElement | HTMLButtonElement | HTMLAnchorElement>) => {
-    onKeyDown && onKeyDown(event)
-
-    if ((event.key === 'Backspace' || event.key === 'Delete') && onRemove) {
-      onRemove()
-    }
-  },
-  'aria-label': onRemove ? `${text}, press backspace or delete to remove` : undefined
-}))<TokenBaseProps & SxProp>`
+const StyledTokenBase = styled.span<SxProp>`
   align-items: center;
   border-radius: 999px;
   cursor: ${props => (isTokenInteractive(props) ? 'pointer' : 'auto')};
   display: inline-flex;
   font-weight: ${get('fontWeights.bold')};
   font-family: inherit;
   text-decoration: none;
+  position: relative;
   white-space: nowrap;
   ${variants}
   ${sx}
 `
 
+const TokenBase = React.forwardRef<TokenElements, TokenBaseProps & SxProp>(
+  ({text, onRemove, onKeyDown, id, ...rest}, forwardedRef) => {
+    return (
+      <StyledTokenBase
+        onKeyDown={(event: KeyboardEvent<TokenElements>) => {
+          onKeyDown && onKeyDown(event)
+
+          if ((event.key === 'Backspace' || event.key === 'Delete') && onRemove) {
+            onRemove()
+          }
+        }}
+        aria-label={onRemove ? `${text}, press backspace or delete to remove` : undefined}
+        id={id?.toString()}
+        {...rest}
+        ref={forwardedRef}
+      />
+    )
+  }
+)
+
 TokenBase.defaultProps = {
   as: 'span',
   size: defaultTokenSize