Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 677
Fix TextInput types#1959
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
Uh oh!
There was an error while loading. Please reload this page.
Fix TextInput types #1959
Changes from all commits
15f9e4babafd9530cf3f6fcb5cd842d96ce2121744a22c7d197777eee88fed1cd292bbea5c974594e842611a84235110f8f0c25902678bbcd4d98cfac292b3File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
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 | ||
| --- | ||
| Fix `TextInput` types |
| 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, | ||
| | 'block' | ||
| | 'contrast' | ||
| | 'disabled' | ||
| | 'monospace' | ||
| | 'sx' | ||
| | 'width' | ||
| | 'maxWidth' | ||
| | 'minWidth' | ||
| | 'variant' | ||
| | 'size' | ||
| | 'validationStatus' | ||
ContributorAuthor 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. This type was missing | ||
| > | ||
| // 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> | ||
ContributorAuthor 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. This line was the root of the problem. Passing a styled component type to importstyledfrom'styled-components'importReactfrom'react'constStyledComponent=styled.div`...`typeProps=React.ComponentPropsWithoutRef<typeofStyledComponent>// anyContributor 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. 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, | ||
| @@ -78,13 +87,12 @@ const TextInput = React.forwardRef<HTMLInputElement, TextInputInternalProps>( | ||
| </TextInputWrapper> | ||
| ) | ||
| } | ||
| ) | ||
| ) as PolymorphicForwardRefComponent<'input', TextInputProps> | ||
ContributorAuthor 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. Using the polymorphic type utility from Radix here to implement correct types for the Eventually, we should deprecate the | ||
| TextInput.defaultProps = { | ||
| type: 'text' | ||
| } | ||
| TextInput.displayName = 'TextInput' | ||
| export type TextInputProps = ComponentProps<typeof TextInput> | ||
| export default TextInput | ||
| 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 | ||
| */ | ||
| @@ -53,7 +52,7 @@ type TextInputWithTokensInternalProps<TokenComponentType extends AnyReactCompone | ||
| * The number of tokens to display before truncating | ||
| */ | ||
| visibleTokenCount?: number | ||
| } & TextInputProps | ||
| } & Omit<TextInputProps, 'size'> | ||
ContributorAuthor 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. The | ||
| const overflowCountFontSizeMap: Record<TokenSizeKeys, number> = { | ||
| small: 0, | ||
| @@ -72,7 +71,6 @@ function TextInputWithTokensInnerComponent<TokenComponentType extends AnyReactCo | ||
| className, | ||
| block, | ||
| disabled, | ||
| theme, | ||
| sx: sxProp, | ||
| tokens, | ||
| onTokenRemove, | ||
| @@ -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>> | ||
| }, | ||
ContributorAuthor 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. I don't think Contributor 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. 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) | ||
| @@ -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} | ||
| @@ -372,5 +366,4 @@ TextInputWithTokens.defaultProps = { | ||
| TextInputWithTokens.displayName = 'TextInputWithTokens' | ||
| export type TextInputWithTokensProps = ComponentProps<typeof TextInputWithTokens> | ||
| export default TextInputWithTokens | ||
| 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" /> | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
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.
ComponentProps<>wasn't necessary here. We should avoid usingComponentProps<>when possible because it can yield unexpected results somtimes.