-
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 7 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,14 +7,14 @@ 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'; | ||
|
||
export type LabelPosition = 'top' | 'bottom' | 'side' | 'edge'; | ||
|
@@ -27,9 +27,24 @@ 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; | ||
isFocused: boolean; | ||
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. Should we also add docs for 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. Sure thing. Does this description look correct? 128618a 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. Yep, looks good! |
||
/** | ||
* The position of the label. | ||
* | ||
* @default 'top' | ||
*/ | ||
labelPosition?: LabelPosition; | ||
/** | ||
* Adjusts the size of the input. | ||
* | ||
* @default 'default' | ||
*/ | ||
size?: Size; | ||
} | ||
|
||
|
@@ -39,30 +54,78 @@ 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 { | ||
export interface InputBaseProps extends BaseProps { | ||
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; | ||
} | ||
|
||
|
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.
I believe this is the correct placement of
FlexProps
. See a682653There 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.
I think that normally we have "merged" prop types in the
types.ts
file (similarly to how the code was before the changes in this PR).Is the reason for this to avoid that
InputControlProps
inheritsFlexProps
fromInputBaseProps
? I wonder if thoseFlexProps
were ever passed toInputControl
(in that case, removing them could be seen as a breaking change?)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.
Ah ok, I'll keep the logic in the types.ts file. 53b7e08
Correct 👍
I verified that any
FlexProps
passed toInputControl
would have been passed through (via...props
) to an<input>
, where they would have no effect. So I think we're ok.