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.

Sign up for GitHub

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

Jump to bottom

Chore: Replace rc-time-picker with rc-picker #99022

Merged
merged 15 commits into from
Jan 16, 2025
Merged

Chore: Replace rc-time-picker with rc-picker #99022

merged 15 commits into from
Jan 16, 2025

Conversation

ashharrison90
Copy link
Contributor

@ashharrison90 ashharrison90 commented Jan 15, 2025 •
edited
Loading

What is this feature?

  • removes rc-time-picker and replaces with the maintained (and compatible with react 19) rc-picker
  • removes the global rcTimePicker styles
  • updates TimeOfDayPicker to use rc-picker
    • adds 2 new props, allowEmpty and placeholder
    • tightens up the types so that when allowEmpty is specified, you're given the correct format for onChange
  • removes the TimePickerInput component
    • this was very similar to the TimeOfDayPicker that we expose in @grafana/ui
    • have incorporated the differences (allowEmpty/placeholder props) into TimeOfDayPicker
  • changes TimeRegionEditor to use TimeOfDayPicker to reduce repetition
  • updates the unit tests in DateTimePicker to work with the new component
before after
image image
image image

Why do we need this feature?

  • reduce repetition
  • update to a maintained library
  • unblocks react 19 upgrade

Who is this feature for?

  • grafana devs

Which issue(s) does this PR fix?:

Fixes #97915

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@ashharrison90 ashharrison90 added type/chore no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Jan 15, 2025
@ashharrison90 ashharrison90 added this to the 11.5.x milestone Jan 15, 2025
@ashharrison90 ashharrison90 self-assigned this Jan 15, 2025
@ashharrison90 ashharrison90 requested review from a team and grafanabot as code owners January 15, 2025 15:10
@ashharrison90 ashharrison90 requested review from tskarhed, Clarity-89, kaydelaney and oshirohugo and removed request for a team January 15, 2025 15:10
Clarity-89
Clarity-89 reviewed Jan 16, 2025
View reviewed changes
Copy link
Contributor

@Clarity-89 Clarity-89 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code changes look good, UI-wise noticed a few issues:

  • The scrollbars are a bit too close to the time display.
    Screenshot 2025-01-16 at 9 15 57
  • There's a lot if empty space at the end of the picker, I guess due to the excluded time slots?
    Screenshot 2025-01-16 at 9 20 41

@ashharrison90
Copy link
Contributor Author

Code changes look good, UI-wise noticed a few issues:

  • The scrollbars are a bit too close to the time display.
    Screenshot 2025-01-16 at 9 15 57
  • There's a lot if empty space at the end of the picker, I guess due to the excluded time slots?
    Screenshot 2025-01-16 at 9 20 41

nice, thanks for looking, i can tweak these. what do you mean by

There's a lot if empty space at the end of the picker

don't really understand it from the screenshot 🤔

@Clarity-89
Copy link
Contributor

There's a lot if empty space at the end of the picker

don't really understand it from the screenshot 🤔

If you open the picker and keep scrolling down, it scrolls way past the last number. In the current picker the scroll stops at the last values:

Screenshot 2025-01-16 at 13 43 10

@ashharrison90
Copy link
Contributor Author

There's a lot if empty space at the end of the picker

don't really understand it from the screenshot 🤔

If you open the picker and keep scrolling down, it scrolls way past the last number. In the current picker the scroll stops at the last values:

Screenshot 2025-01-16 at 13 43 10

ah gotcha, will fix that as well 🙇

@ashharrison90
Copy link
Contributor Author

should be all fixed @Clarity-89 👍

Clarity-89
Clarity-89 approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@Clarity-89 Clarity-89 left a comment

Choose a reason for hiding this comment

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

Great work! 👏
There's a layout shift when scrollbar appears, but not sure how easy that is to fix.

@ashharrison90
Copy link
Contributor Author

you able to share a video of that behaviour? 🙏

@Clarity-89
Copy link
Contributor

you able to share a video of that behaviour? 🙏

Screen.Recording.2025-01-16.at.14.42.30.mov

@ashharrison90
Copy link
Contributor Author

you able to share a video of that behaviour? 🙏

Screen.Recording.2025-01-16.at.14.42.30.mov

fixed that as well, thanks 👍

@ashharrison90 ashharrison90 merged commit cff07c9 into main Jan 16, 2025
17 checks passed
@ashharrison90 ashharrison90 deleted the ash/replace-rc-time-picker branch January 16, 2025 15:58
owensmallwood pushed a commit that referenced this pull request Jan 16, 2025
@ashharrison90 @owensmallwood
Chore: Replace rc-time-picker with rc-picker (#99022)
6aaac8e 
* replace rc-time-picker with rc-picker

* remove unnecessary stack

* remove TimePickerInput

* make props stricter

* fix type assertions

* remove outdated comment

* fix types

* fix clear icon

* fix styling

* fix z-index and 6 tests

* fix remaining unit tests

* betterer results

* styling tweaks

* don't show/hide scrollbars on hover
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Clarity-89 Clarity-89 Clarity-89 approved these changes

@grafanabot grafanabot Awaiting requested review from grafanabot grafanabot is a code owner

@tskarhed tskarhed Awaiting requested review from tskarhed tskarhed is a code owner automatically assigned from grafana/grafana-frontend-platform

@kaydelaney kaydelaney Awaiting requested review from kaydelaney kaydelaney is a code owner automatically assigned from grafana/frontend-ops

@oshirohugo oshirohugo Awaiting requested review from oshirohugo oshirohugo is a code owner automatically assigned from grafana/plugins-platform-frontend

Assignees

@ashharrison90 ashharrison90

Labels
area/frontend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes type/chore
Projects
None yet
Milestone
11.5.x
Development

Successfully merging this pull request may close these issues.

Chore: Replace rc-time-picker to unblock React 19 upgrade
2 participants
@ashharrison90 @Clarity-89