-
Notifications
You must be signed in to change notification settings - Fork 534
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
Fix TextInput types #1959
Changes from 16 commits
15f9e4b
abafd95
30cf3f6
fcb5cd8
42d96ce
2121744
a22c7d1
97777ee
e88fed1
cd292bb
ea5c974
594e842
611a842
35110f8
f0c2590
2678bbc
d4d98cf
ac292b3
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 | ||
--- | ||
|
||
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' | ||
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> | ||
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 import styled from 'styled-components'
import React from 'react'
const StyledComponent = styled.div`...`
type Props =
React.ComponentPropsWithoutRef<typeof StyledComponent>
// any 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> | ||
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'> | ||
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>> | ||
}, | ||
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 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" /> | ||
} |
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.