Skip to content
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

ui: Improve time picker keyboard input #81200

Merged
merged 1 commit into from
May 18, 2022

Conversation

jocrl
Copy link
Contributor

@jocrl jocrl commented May 11, 2022

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, 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:
    Keyboard friendly DatePicker component 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. 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
    (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. 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.
    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)"

@jocrl jocrl requested a review from a team May 11, 2022 20:19
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jocrl jocrl force-pushed the time-picker-text-entry branch from f4b394d to ac27006 Compare May 12, 2022 14:11
Copy link
Contributor

@matthewtodd matthewtodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good, simple improvement. :lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: :shipit: complete! 1 of 0 LGTMs obtained

@jocrl jocrl force-pushed the time-picker-text-entry branch from ac27006 to 91ab2f7 Compare May 16, 2022 20:16
@jocrl jocrl requested a review from a team May 16, 2022 20:16
@jocrl jocrl requested review from a team as code owners May 16, 2022 20:16
@jocrl jocrl requested review from a team May 16, 2022 20:16
@jocrl jocrl requested review from a team as code owners May 16, 2022 20:16
@jocrl jocrl requested review from stevendanna and removed request for a team May 16, 2022 20:16
@jocrl jocrl force-pushed the time-picker-text-entry branch from 91ab2f7 to 0f1328c Compare May 16, 2022 20:19
@jocrl
Copy link
Contributor Author

jocrl commented May 16, 2022

Yup! Rebased, thanks 🙂

@otan otan removed request for a team May 16, 2022 23:15
@otan otan removed request for a team and stevendanna May 16, 2022 23:15
@tbg
Copy link
Member

tbg commented May 17, 2022

Nice! I was about to say that the p thing wasn't a lot better than before (thanks for looking into it - what a pain). But I assume now that we've switched to 24h time, that oddity is gone and things actually work as one would expect?

@jocrl
Copy link
Contributor Author

jocrl commented May 17, 2022

Nice! I was about to say that the p thing wasn't a lot better than before (thanks for looking into it - what a pain). But I assume now that we've switched to 24h time, that oddity is gone and things actually work as one would expect?

Yup, thanks! Both 03:03 and 3:03 will work. Will update the description.

@jocrl jocrl force-pushed the time-picker-text-entry branch 2 times, most recently from ff6a252 to 5791e22 Compare May 17, 2022 20:01
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)"`
@jocrl jocrl force-pushed the time-picker-text-entry branch from 5791e22 to 88b245a Compare May 17, 2022 20:01
@jocrl
Copy link
Contributor Author

jocrl commented May 18, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented May 18, 2022

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui: Allow text entry in the time selector
5 participants