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

release-22.1: improvements on timepicker #82495

Merged

Conversation

maryliag
Copy link
Contributor

@maryliag maryliag commented Jun 6, 2022

Backport 1/1 commits from #81200 on behalf of @jocrl
Backport 1/1 commits from #81464 on behalf of @jocrl
Backport 1/1 commits from #81855 on behalf of @jocrl

/cc @cockroachdb/release


ui: Improve time picker keyboard input
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.

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)"


ui: Add tests for the TimeScaleDropdown component
This commit splits out tests in timescale.spec.tsx and adds additional tests
for the TimeScaleDropdown component, including testing that the custom time
picker is initialized to the currently selected time. This also adds
TimeScaleDropdown stories.

Release note: None


ui: open TimeScaleDropdown directly to the custom menu when custom is selected

Fixes #78499

This commit modifies the TimeScaleDropdown so that if a custom time is currently
select it opens directly to the custom selection menu, and a "Preset Time
Ranges" navigation link to go to the preset options from the custom menu.

This commit also cleans up the unused DateRange component.

Release note (ui): The time picker now opens directly to the custom time
selection menu when a custom time is already selected. A "Preset Time Ranges"
navigation link has been added to go to the preset options from the custom
menu.


Release justification: low risk, high benefit changes

jocrl added 3 commits June 6, 2022 18:19
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)"`
This commit splits out tests in `timescale.spec.tsx` and adds additional tests
for the TimeScaleDropdown component, including testing that the custom time
picker is initialized to the currently selected time. This also adds
TimeScaleDropdown stories.

Release note: None
… selected

Fixes cockroachdb#78499

This commit modifies the TimeScaleDropdown so that if a custom time is currently
select it opens directly to the custom selection menu, and a "Preset Time
Ranges" navigation link to go to the preset options from the custom menu.

This commit also cleans up the unused DateRange component.

Release note (ui): The time picker now opens directly to the custom time
selection menu when a custom time is already selected. A "Preset Time Ranges"
navigation link has been added to go to the preset options from the custom
menu.
@maryliag maryliag requested a review from a team June 6, 2022 22:29
@blathers-crl
Copy link

blathers-crl bot commented Jun 6, 2022

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

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.

None yet

4 participants