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

SceneTimeRange: Implement onZoom behavior #374

Merged
merged 9 commits into from
Oct 3, 2023
Merged

SceneTimeRange: Implement onZoom behavior #374

merged 9 commits into from
Oct 3, 2023

Conversation

polibb
Copy link
Contributor

@polibb polibb commented Sep 28, 2023 •
edited by grafanabot
Loading

Zoom-out button in SceneTimePicker has an effect on the time range.

To do:

  • tests

This implementation only handles zooming out,
I'm not sure what to do with the refresh and setting of the URL that happen on zoom-out in core: https://github.com/grafana/grafana/blob/main/public/app/features/dashboard/services/TimeSrv.ts#L290.

Fixes #299

📦 Published PR as canary version: 1.11.2--canary.374.6394808989.0

✨ Test out this PR locally via:

npm install @grafana/[email protected]
# or 
yarn add @grafana/[email protected]

@polibb polibb added the type/enhancement New feature or request label Sep 28, 2023
@polibb polibb self-assigned this Sep 28, 2023
@polibb polibb requested a review from dprokop September 28, 2023 14:44
@CLAassistant
Copy link

CLAassistant commented Sep 28, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

torkelo
torkelo reviewed Sep 29, 2023
View reviewed changes
Copy link
Member

@torkelo torkelo 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.

Looks super good and close to mergable 🎉 just needs 1-2 tests for the zoom out logic (which you can probably copy from core)

@polibb polibb marked this pull request as ready for review October 2, 2023 14:50
@polibb polibb added patch Increment the patch version when merged release Create a release when this pr is merged labels Oct 2, 2023
torkelo
torkelo approved these changes Oct 2, 2023
View reviewed changes
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Very close! just need to remove the useState (line 20)

packages/scenes/src/components/SceneTimePicker.tsx Outdated Show resolved Hide resolved
dprokop
dprokop approved these changes Oct 3, 2023
View reviewed changes
Copy link
Member

@dprokop dprokop left a comment

Choose a reason for hiding this comment

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

This is looking good @polibb! Think one last comment from Torkel is there to be addressed and we should be good to merge :)

packages/scenes/src/components/SceneTimePicker.tsx Outdated Show resolved Hide resolved
@polibb polibb merged commit 3102df5 into main Oct 3, 2023
@polibb polibb deleted the polibb/zoom-out-timerange branch October 3, 2023 15:11
@grafanabot
Copy link
Contributor

🚀 PR was released in v1.12.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dprokop dprokop dprokop approved these changes

@torkelo torkelo torkelo approved these changes

@axelavargas axelavargas Awaiting requested review from axelavargas

Assignees

@polibb polibb

Labels
patch Increment the patch version when merged release Create a release when this pr is merged released type/enhancement New feature or request
Projects
Dashboards
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SceneTimeRange: Implement onZoom behavior
5 participants
@polibb @CLAassistant @grafanabot @torkelo @dprokop