ui: Improve time picker keyboard input#81200
Conversation
f4b394d to
ac27006
Compare
matthewtodd
left a comment
There was a problem hiding this comment.
This seems like a good, simple improvement.
Reviewable status:
complete! 1 of 0 LGTMs obtained
maryliag
left a comment
There was a problem hiding this comment.
Should you wait for #81077 to be merged or even rebase on top of it? Because that PR changes to a 24h format, so you won't need to input p or P for example, it would be just the number directly (on a 24h format)
Reviewable status:
complete! 1 of 0 LGTMs obtained
ac27006 to
91ab2f7
Compare
91ab2f7 to
0f1328c
Compare
|
Yup! Rebased, thanks 🙂 |
|
Nice! I was about to say that the |
Yup, thanks! Both |
ff6a252 to
5791e22
Compare
Fixes cockroachdb#80655, mostly. Previously, users had to type `1:03 PM (UTC)` in order to enter text into the time picker. This commit modifies the time picker so that users would instead type either - `1:03`, or - `01:03` to enter text into the time picker. Copying and re-pasting the text from a time picker still works. Alternate approaches not pursued (these are not needed with the removal of AM/PM). 1) Make our own time picker component, and style it to look like antd's. There's a general issue of antd's components not being keyboard friendly: ant-design/ant-design#5685 2) Upgrade to `antd` version 4, and use an undocumented prop type. `antd`'s time picker uses the time picker from the `rc-picker` library under the hood. In `rc-picker`, the `format` prop is of type `String | String[]`, where if it's an array the first value will be used for display and the remaining ones will be used for parsing, as documented [here] (https://react-component.github.io/picker). In theory, `antd` passes `format` (and also any remaining, additional props in addition to the specified ones) to the `rc-picker` component. So even though the `antd` `TimePicker` component `format` prop is not documented to take in a string array, one might think that passing in a string array anyway would work. In practice, passing in a string array works in `antd` version `4.20.4`, as tested in the [antd sandbox] (https://ant.design/docs/react/getting-started) (render `<TimePicker format={ ["h:mm A", "h:mma"]}/>`). However, this does not work in our codebase (which installs `antd` `v3.25.2`), or in the `antd` version `3.x` [sandbox] (https://3x.ant.design/docs/react/getting-started). The errors that appear in these two situations are different, and in a way demonstrate the potential for breakage from using an undocumented feature in future upgrades from a version that we've to work. If we do this, we should add a test. 3) Dead end: The `antd` `TimePicker` component takes an `onChange` prop with a second `timeString` paramater. However, `onChange` only fires if the input is of the correct, parsable format. The specific code that ignores text input that is not of a parsable format is in `rc-picker`, [here] (https://github.com/react-component/picker/blob/5306c8938aded49c5d6f6b6d4761ddab25e3cce9/src/Picker.tsx#L237). This prevents us from being the one to do the fuzzy matching and passing the value back to the component. Release note (ui): The time picker component has been improved such that users can use keyboard input to select a time without having to type `" (UTC)"`
5791e22 to
88b245a
Compare
|
bors r+ |
|
Build succeeded: |
Fixes #80655, mostly.
Previously, users had to type
1:03 PM (UTC)in order to enter text into the time picker.This commit modifies the time picker so that users would instead type either
1:03, or01:03to enter text into the time picker. Copying and re-pasting the text from a time picker still works.
Alternate approaches not pursued (these are not needed with the removal of AM/PM).
Make our own time picker component, and style it to look like antd's. There's
a general issue of antd's components not being keyboard friendly:
Keyboard friendly DatePicker component ant-design/ant-design#5685
Upgrade to
antdversion 4, and use an undocumented prop type.antd's timepicker uses the time picker from the
rc-pickerlibrary under the hood. Inrc-picker, theformatprop is of typeString | String[], where if it's anarray the first value will be used for display and the remaining ones will be
used for parsing, as documented here. In theory,
antdpassesformat(and also any remaining, additional props in addition to the specified ones) to
the
rc-pickercomponent. So even though theantdTimePickercomponentformatprop is not documented to take in a string array, one might think thatpassing in a string array anyway would work. In practice, passing in a string
array works in
antdversion4.20.4, as tested in the antd sandbox(render
<TimePicker format={ ["h:mm A", "h:mma"]}/>). However, this does not work in our codebase(which installs
antdv3.25.2), or in theantdversion3.xsandbox. The errors that appear inthese two situations are different, and in a way demonstrate the potential for
breakage from using an undocumented feature in future upgrades from a version
that we've to work. If we do this, we should add a test.
Dead end: The
antdTimePickercomponent takes anonChangeprop with asecond
timeStringparamater. However,onChangeonly fires if the input isof the correct, parsable format. The specific code that ignores text input that
is not of a parsable format is in
rc-picker, here.This prevents us from being the one to do the fuzzy matching and passing the
value back to the component.
Release note (ui): The time picker component has been improved such that users
can use keyboard input to select a time without having to type
" (UTC)"