Skip to content

Commit

Permalink
ui: Improve time picker keyboard input
Browse files Browse the repository at this point in the history
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 p`, or
- `1:03 P`

to enter text into the time picker. Copying and re-pasting the text from a time picker still works.

Typing `1:03pm`, `1:03PM`, or other versions without the space before `PM` still do not
work.

Additionally, typing the keys in `1:03 pm` will lead to the input being `1:03 PMm`, as the
`M` autofill after `p` is typed. The `1:03 PMm` input in the text box is still
accepted, but it does look weird and will likely be annoying to users who will
delete the trailing `m`.

Alternate approaches not pursued

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)"`
  • Loading branch information
jocrl committed May 12, 2022
1 parent bc1ee7c commit ac27006
Showing 1 changed file with 3 additions and 3 deletions.
6 changes: 3 additions & 3 deletions pkg/ui/workspaces/cluster-ui/src/dateRange/dateRange.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export function DateRangeMenu({
onCancel,
}: DateRangeMenuProps): React.ReactElement {
const dateFormat = "MMMM D, YYYY";
const timeFormat = "h:mm A [(UTC)]";
const timeFormat = "h:mm A";
const [startMoment, setStartMoment] = useState<Moment>(
startInit || moment.utc(),
);
Expand Down Expand Up @@ -108,7 +108,7 @@ export function DateRangeMenu({
return (
<div className={cx("popup-content")}>
<Text className={cx("label")} textType={TextTypes.BodyStrong}>
Start
Start (UTC)
</Text>
<DatePicker
disabledDate={isDisabled}
Expand All @@ -128,7 +128,7 @@ export function DateRangeMenu({
/>
<div className={cx("divider")} />
<Text className={cx("label")} textType={TextTypes.BodyStrong}>
End
End (UTC)
</Text>
<DatePicker
allowClear={false}
Expand Down

0 comments on commit ac27006

Please sign in to comment.