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

[Maps] Should be able to zoom-in on selected range of timeslider #122131

Merged
merged 5 commits into from
Jan 25, 2022

Conversation

maksimkovalev
Copy link
Contributor

Closes: #105596

This PR contains changes for the time slider component. Users can now set Kibana's global time range according to the selected time slice in the timeslider component by clicking calendar button.

Screenshot 2021-12-29 at 14 39 55

cc @thomasneirynck

@kibanamachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Dec 29, 2021

💚 CLA has been signed

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

This looks great.

I'd make one addition: set the timeslice to the entire range as well.

Otherwise the data "blips".

e.g.
Step 1) open timeslider
image

Step 2) click "zoom in" calendar button

image

^ note that the seleted timeslice is expanded so it is identical to the previous timeslice. This way, the data on-screen remains identical.

@thomasneirynck thomasneirynck added release_note:enhancement [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.1.0 labels Jan 4, 2022
@maksimkovalev
Copy link
Contributor Author

@thomasneirynck What if we change the current behavior of auto-select from:

componentDidMount() {
    this._isMounted = true;
    // auto-select range between first tick and second tick
    this._onChange([this.state.ticks[0].value, this.state.ticks[1].value]);
}

to

componentDidMount() {
    this._isMounted = true;
    this._onChange([this.state.min, this.state.max]);
}

This looks like a more appropriate solution here, because it also fixes the current behavior of the initial opening of the timeslider. This will allow you to get the entire period of the global time window, not just the range between the first and second ticks, and it will not change the data on the map.

@thomasneirynck
Copy link
Contributor

discussed with offline with @nreese @maksimkovalev #122131 (comment)

We can discard this and keep the current behavior. We can keep #122131 (review) as a separate improvement.

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

thx!

@thomasneirynck thomasneirynck marked this pull request as ready for review January 10, 2022 16:47
@thomasneirynck thomasneirynck requested review from a team as code owners January 10, 2022 16:47
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@nreese nreese requested review from gvnmagni and removed request for miukimiu January 12, 2022 14:12
@thomasneirynck
Copy link
Contributor

@elasticmachine merge upstream

@thomasneirynck
Copy link
Contributor

buildkite test this

@maksimkovalev
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
maps 2.5MB 2.5MB +633.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
maps 37.1KB 37.2KB +50.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@nreese nreese merged commit 10c96e5 into elastic:main Jan 25, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 25, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting 💝community [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:enhancement v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] Should be able to zoom-in on selected range of timeslider
8 participants