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

Fix TextInput types #1959

Merged
merged 18 commits into from
Mar 16, 2022
Merged
Show file tree
Hide file tree
Changes from 16 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/loud-schools-own.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

Fix `TextInput` types
2 changes: 1 addition & 1 deletion src/Autocomplete/AutocompleteInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ const AutocompleteInput = React.forwardRef(
aria-expanded={showMenu}
aria-haspopup="listbox"
aria-owns={`${id}-listbox`}
autocomplete="off"
autoComplete="off"
id={id}
{...props}
/>
Expand Down
28 changes: 18 additions & 10 deletions src/TextInput.tsx
Original file line number Diff line number Diff line change
@@ -1,25 +1,34 @@
import {ForwardRefComponent as PolymorphicForwardRefComponent} from '@radix-ui/react-polymorphic'
import classnames from 'classnames'
import React from 'react'
import {ComponentProps, Merge} from './utils/types'
import {Merge} from './utils/types'
import TextInputWrapper, {StyledWrapperProps} from './_TextInputWrapper'
import UnstyledTextInput from './_UnstyledTextInput'
import TextInputWrapper from './_TextInputWrapper'

type NonPassthroughProps = {
className?: string
/** @deprecated Use `leadingVisual` or `trailingVisual` prop instead */
icon?: React.ComponentType<{className?: string}>
leadingVisual?: string | React.ComponentType<{className?: string}>
trailingVisual?: string | React.ComponentType<{className?: string}>
} & Pick<
ComponentProps<typeof TextInputWrapper>,
'block' | 'contrast' | 'disabled' | 'monospace' | 'sx' | 'width' | 'maxWidth' | 'minWidth' | 'variant' | 'size'
StyledWrapperProps,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ComponentProps<> wasn't necessary here. We should avoid using ComponentProps<> when possible because it can yield unexpected results somtimes.

| 'block'
| 'contrast'
| 'disabled'
| 'monospace'
| 'sx'
| 'width'
| 'maxWidth'
| 'minWidth'
| 'variant'
| 'size'
| 'validationStatus'
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 type was missing validationStatus

>

// Note: using ComponentProps instead of ComponentPropsWithoutRef here would cause a type issue where `css` is a required prop.
type TextInputInternalProps = Merge<React.ComponentPropsWithoutRef<typeof UnstyledTextInput>, NonPassthroughProps>
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 line was the root of the problem. Passing a styled component type to React.ComponentPropsWithoutRef yields an any type

import styled from 'styled-components'
import React from 'react'

const StyledComponent = styled.div`...`

type Props = 
  React.ComponentPropsWithoutRef<typeof StyledComponent>
// any

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to know so we don't make this mistake in the future.

export type TextInputProps = Merge<React.ComponentPropsWithoutRef<'input'>, NonPassthroughProps>

// using forwardRef is important so that other components (ex. SelectMenu) can autofocus the input
const TextInput = React.forwardRef<HTMLInputElement, TextInputInternalProps>(
const TextInput = React.forwardRef<HTMLInputElement, TextInputProps>(
(
{
icon: IconComponent,
Expand Down Expand Up @@ -78,13 +87,12 @@ const TextInput = React.forwardRef<HTMLInputElement, TextInputInternalProps>(
</TextInputWrapper>
)
}
)
) as PolymorphicForwardRefComponent<'input', TextInputProps>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the polymorphic type utility from Radix here to implement correct types for the as prop.

Eventually, we should deprecate the as prop for TextInput. I can't think of a good use case for it now that we have the Textarea component


TextInput.defaultProps = {
type: 'text'
}

TextInput.displayName = 'TextInput'

export type TextInputProps = ComponentProps<typeof TextInput>
export default TextInput
29 changes: 11 additions & 18 deletions src/TextInputWithTokens.tsx
Original file line number Diff line number Diff line change
@@ -1,25 +1,24 @@
import React, {FocusEventHandler, KeyboardEventHandler, MouseEventHandler, RefObject, useRef, useState} from 'react'
import {omit} from '@styled-system/props'
import {FocusKeys} from '@primer/behaviors'
import {isFocusable} from '@primer/behaviors/utils'
import {omit} from '@styled-system/props'
import React, {FocusEventHandler, KeyboardEventHandler, MouseEventHandler, RefObject, useRef, useState} from 'react'
import Box from './Box'
import {useProvidedRefOrCreate} from './hooks'
import {useCombinedRefs} from './hooks/useCombinedRefs'
import {useFocusZone} from './hooks/useFocusZone'
import {ComponentProps} from './utils/types'
import Text from './Text'
import {TextInputProps} from './TextInput'
import Token from './Token/Token'
import {TokenSizeKeys} from './Token/TokenBase'
import {TextInputProps} from './TextInput'
import {useProvidedRefOrCreate} from './hooks'
import UnstyledTextInput from './_UnstyledTextInput'
import TextInputWrapper, {textInputHorizPadding, TextInputSizes} from './_TextInputWrapper'
import Box from './Box'
import Text from './Text'
import {isFocusable} from '@primer/behaviors/utils'
import UnstyledTextInput from './_UnstyledTextInput'

// eslint-disable-next-line @typescript-eslint/no-explicit-any
type AnyReactComponent = React.ComponentType<any>

// NOTE: if these props or their JSDoc comments are updated, be sure to also update
// the prop table in docs/content/TextInputTokens.mdx
type TextInputWithTokensInternalProps<TokenComponentType extends AnyReactComponent> = {
export type TextInputWithTokensProps<TokenComponentType extends AnyReactComponent = typeof Token> = {
/**
* The array of tokens to render
*/
Expand Down Expand Up @@ -53,7 +52,7 @@ type TextInputWithTokensInternalProps<TokenComponentType extends AnyReactCompone
* The number of tokens to display before truncating
*/
visibleTokenCount?: number
} & TextInputProps
} & Omit<TextInputProps, 'size'>
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 size prop from TextInputProps was clashing with the size prop defined above


const overflowCountFontSizeMap: Record<TokenSizeKeys, number> = {
small: 0,
Expand All @@ -72,7 +71,6 @@ function TextInputWithTokensInnerComponent<TokenComponentType extends AnyReactCo
className,
block,
disabled,
theme,
sx: sxProp,
tokens,
onTokenRemove,
Expand All @@ -88,10 +86,7 @@ function TextInputWithTokensInnerComponent<TokenComponentType extends AnyReactCo
variant: variantProp, // deprecated. use `size` instead
visibleTokenCount,
...rest
}: TextInputWithTokensInternalProps<TokenComponentType> & {
selectedTokenIndex: number | undefined
setSelectedTokenIndex: React.Dispatch<React.SetStateAction<number | undefined>>
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think selectedTokenIndex and setSelectedTokenIndex are props that we use in this component

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. I'm not sure how these types made it here 🤔

}: TextInputWithTokensProps<TokenComponentType>,
externalRef: React.ForwardedRef<HTMLInputElement>
) {
const {onBlur, onFocus, onKeyDown, ...inputPropsRest} = omit(rest)
Expand Down Expand Up @@ -252,7 +247,6 @@ function TextInputWithTokensInnerComponent<TokenComponentType extends AnyReactCo
disabled={disabled}
hasLeadingVisual={Boolean(LeadingVisual)}
hasTrailingVisual={Boolean(TrailingVisual)}
theme={theme}
width={widthProp}
minWidth={minWidthProp}
maxWidth={maxWidthProp}
Expand Down Expand Up @@ -372,5 +366,4 @@ TextInputWithTokens.defaultProps = {

TextInputWithTokens.displayName = 'TextInputWithTokens'

export type TextInputWithTokensProps = ComponentProps<typeof TextInputWithTokens>
export default TextInputWithTokens
2 changes: 1 addition & 1 deletion src/__tests__/FormControl.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ describe('FormControl', () => {
{text: 'one', id: 1},
{text: 'two', id: 2}
]}
onRemove={onRemoveMock}
onTokenRemove={onRemoveMock}
/>
</FormControl>
</SSRProvider>
Expand Down
2 changes: 1 addition & 1 deletion src/__tests__/SelectMenu.types.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export function shouldNotAcceptSystemProps() {
<SelectMenu.List backgroundColor="lightgreen" />
{/* @ts-expect-error system props should not be accepted */}
<SelectMenu.Divider backgroundColor="lightgrey" />
{/* This will not error for now, but once TextInputProps is fixed, a ts-expect-error can be added */}
{/* @ts-expect-error system props should not be accepted */}
<SelectMenu.Filter backgroundColor="lightpink" />
{/* @ts-expect-error system props should not be accepted */}
<SelectMenu.Footer backgroundColor="lightsalmon" />
Expand Down
4 changes: 2 additions & 2 deletions src/__tests__/TextInput.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,11 @@ describe('TextInput', () => {
})

it('renders warning', () => {
expect(render(<TextInput name="zipcode" status="warning" />)).toMatchSnapshot()
expect(render(<TextInput name="zipcode" validationStatus="warning" />)).toMatchSnapshot()
})

it('renders error', () => {
expect(render(<TextInput name="zipcode" status="error" />)).toMatchSnapshot()
expect(render(<TextInput name="zipcode" validationStatus="error" />)).toMatchSnapshot()
})

it('renders contrast', () => {
Expand Down
12 changes: 12 additions & 0 deletions src/__tests__/TextInput.types.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import React from 'react'
import {TextInput} from '..'

export function shouldNotAcceptInvalidDomProps() {
// @ts-expect-error invalid DOM props should not be accepted
return <TextInput onKeyDown={true} />
}

export function shouldNotAcceptInvalidSize() {
// @ts-expect-error invalid size value should not be accepted
return <TextInput size="big" />
}
Loading