-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
InputControl: Improve type annotations and stories #40119
Changes from all commits
963b50d
42ddd30
a682653
a2b6391
b80fdc4
9086a93
107011a
53b7e08
128618a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import type { ComponentMeta, ComponentStory } from '@storybook/react'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import InputControl from '..'; | ||
|
||
const meta: ComponentMeta< typeof InputControl > = { | ||
title: 'Components (Experimental)/InputControl', | ||
component: InputControl, | ||
argTypes: { | ||
__unstableInputWidth: { control: { type: 'text' } }, | ||
__unstableStateReducer: { control: { type: null } }, | ||
onChange: { control: { type: null } }, | ||
prefix: { control: { type: null } }, | ||
suffix: { control: { type: null } }, | ||
type: { control: { type: 'text' } }, | ||
}, | ||
parameters: { | ||
controls: { expanded: true }, | ||
docs: { source: { state: 'open' } }, | ||
}, | ||
}; | ||
export default meta; | ||
|
||
const Template: ComponentStory< typeof InputControl > = ( args ) => ( | ||
<InputControl { ...args } /> | ||
); | ||
|
||
export const Default = Template.bind( {} ); | ||
Default.args = { | ||
label: 'Value', | ||
placeholder: 'Placeholder', | ||
value: '', | ||
}; | ||
|
||
export const WithPrefix = Template.bind( {} ); | ||
WithPrefix.args = { | ||
...Default.args, | ||
prefix: <span style={ { paddingLeft: 8 } }>@</span>, | ||
}; | ||
|
||
export const WithSuffix = Template.bind( {} ); | ||
WithSuffix.args = { | ||
...Default.args, | ||
suffix: <button style={ { marginRight: 4 } }>Send</button>, | ||
}; | ||
|
||
export const WithSideLabel = Template.bind( {} ); | ||
WithSideLabel.args = { | ||
...Default.args, | ||
labelPosition: 'side', | ||
}; | ||
|
||
export const WithEdgeLabel = Template.bind( {} ); | ||
WithEdgeLabel.args = { | ||
...Default.args, | ||
__unstableInputWidth: '20em', | ||
labelPosition: 'edge', | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,15 +7,16 @@ import type { | |
ChangeEvent, | ||
SyntheticEvent, | ||
PointerEvent, | ||
HTMLInputTypeAttribute, | ||
} from 'react'; | ||
import type { useDrag } from '@use-gesture/react'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import type { StateReducer } from './reducer/state'; | ||
import type { FlexProps } from '../flex/types'; | ||
import type { WordPressComponentProps } from '../ui/context'; | ||
import type { FlexProps } from '../flex/types'; | ||
|
||
export type LabelPosition = 'top' | 'bottom' | 'side' | 'edge'; | ||
|
||
|
@@ -27,9 +28,31 @@ export type Size = 'default' | 'small' | '__unstable-large'; | |
|
||
interface BaseProps { | ||
__unstableInputWidth?: CSSProperties[ 'width' ]; | ||
/** | ||
* If true, the label will only be visible to screen readers. | ||
* | ||
* @default false | ||
*/ | ||
hideLabelFromVision?: boolean; | ||
/** | ||
* Whether the component should be in a focused state. | ||
* Used to coordinate focus states when the actual focused element and the component handling | ||
* visual focus are separate. | ||
* | ||
* @default false | ||
*/ | ||
isFocused: boolean; | ||
/** | ||
* The position of the label. | ||
* | ||
* @default 'top' | ||
*/ | ||
labelPosition?: LabelPosition; | ||
/** | ||
* Adjusts the size of the input. | ||
* | ||
* @default 'default' | ||
*/ | ||
size?: Size; | ||
} | ||
|
||
|
@@ -39,35 +62,83 @@ export type InputChangeCallback< | |
> = ( nextValue: string | undefined, extra: { event: E } & P ) => void; | ||
|
||
export interface InputFieldProps extends BaseProps { | ||
/** | ||
* Determines the drag axis. | ||
* | ||
* @default 'n' | ||
*/ | ||
dragDirection?: DragDirection; | ||
/** | ||
* If `isDragEnabled` is true, this controls the amount of `px` to have been dragged before | ||
* the drag gesture is actually triggered. | ||
* | ||
* @default 10 | ||
*/ | ||
dragThreshold?: number; | ||
/** | ||
* If true, enables mouse drag gestures. | ||
* | ||
* @default false | ||
*/ | ||
isDragEnabled?: boolean; | ||
/** | ||
* If true, the `ENTER` key press is required in order to trigger an `onChange`. | ||
* If enabled, a change is also triggered when tabbing away (`onBlur`). | ||
* | ||
* @default false | ||
*/ | ||
isPressEnterToChange?: boolean; | ||
/** | ||
* A function that receives the value of the input. | ||
*/ | ||
onChange?: InputChangeCallback; | ||
onValidate?: ( | ||
nextValue: string, | ||
event?: SyntheticEvent< HTMLInputElement > | ||
) => void; | ||
setIsFocused: ( isFocused: boolean ) => void; | ||
stateReducer?: StateReducer; | ||
/** | ||
* The current value of the input. | ||
*/ | ||
value?: string; | ||
onDragEnd?: ( dragProps: DragProps ) => void; | ||
onDragStart?: ( dragProps: DragProps ) => void; | ||
onDrag?: ( dragProps: DragProps ) => void; | ||
Comment on lines
105
to
107
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. We're lacking in documentation for the drag stuff overall 😓 I'd say it's low priority though. 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. Yeah, that's not an easy one because we want to be careful to document it in a way that doesn't necessarily expose 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. Currently the generated type for these props is a bit convoluted ( 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. Honestly I don't know what that "simplified" version would be 😅 I'm fine with keeping it as is for now, because it's probably irrelevant for 99% of consumers, at least without proper documentation. Also TIL you can override type info in the args table. Nice option to have (when it's worth the maintenance cost)! 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. Agreed, let's keep it as-is and improve it later if/when necessary
That's very useful, definitely good to know! |
||
/** | ||
* Type of the input element to render. | ||
* | ||
* @default 'text' | ||
*/ | ||
type?: HTMLInputTypeAttribute; | ||
} | ||
|
||
export interface InputBaseProps extends BaseProps, FlexProps { | ||
children: ReactNode; | ||
/** | ||
* Renders an element on the left side of the input. | ||
*/ | ||
prefix?: ReactNode; | ||
/** | ||
* Renders an element on the right side of the input. | ||
*/ | ||
suffix?: ReactNode; | ||
/** | ||
* If true, the `input` will be disabled. | ||
* | ||
* @default false | ||
*/ | ||
disabled?: boolean; | ||
className?: string; | ||
id?: string; | ||
/** | ||
* If this property is added, a label will be generated using label property as the content. | ||
*/ | ||
label?: ReactNode; | ||
} | ||
|
||
export interface InputControlProps | ||
extends Omit< InputBaseProps, 'children' | 'isFocused' >, | ||
extends Omit< InputBaseProps, 'children' | 'isFocused' | keyof FlexProps >, | ||
/** | ||
* The `prefix` prop in `WordPressComponentProps< InputFieldProps, 'input', false >` comes from the | ||
* `HTMLInputAttributes` and clashes with the one from `InputBaseProps`. So we have to omit it from | ||
|
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.
Should we also add docs for
isFocused
(including thefalse
default value) ?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.
Sure thing. Does this description look correct? 128618a
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.
Yep, looks good!