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] Move tooltips to store #32333

Merged
merged 13 commits into from
Mar 7, 2019

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented Mar 1, 2019

This is an internal refactor:

  • move tooltip management out of layers, and to the mapbox-component
  • use global handler iso of multiple handlers on individual layers (this did remove the cursor-pointer change, since we no longer are explicitly handling on-enter/leave events).
  • put tooltip state in store (imo - the shape of this state is not super-important just yet. subsequent PRs should develop this further as more functionality gets dialed in)

This needs to fix some bugs

  • when layer is removed, any corresponding tooltip should be removed as well
  • when layer is made invisible, any correspoinding tooltip should be removed as well

Separate PRs will be submitted to extend tooltip functionality, which should include:

  • "lock" tooltip on the map on click (imho, would introduce "pointer"-cursor here. so we can indicate users they can "lock" the tooltip in place).
  • show multiple underlying features in the tooltip
  • highlight underlying feature

tmp commit

tmp commit

remove usage from vector layer
@thomasneirynck thomasneirynck added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 v7.2.0 labels Mar 1, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@thomasneirynck thomasneirynck added the WIP Work in progress label Mar 2, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@thomasneirynck thomasneirynck requested review from kindsun and nreese March 5, 2019 04:50
@thomasneirynck thomasneirynck removed the WIP Work in progress label Mar 5, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@thomasneirynck
Copy link
Contributor Author

jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@thomasneirynck
Copy link
Contributor Author

wrt. #32560 . I'd like to do this in separate PR. I don't want to clutter this too much.

Copy link
Contributor

@nreese nreese 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 a really nice change. It feels very logical to move the mapbox tooltip stuff out of vector_layer and into mb/view

x-pack/plugins/maps/public/components/map/mb/index.js Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/components/map/mb/view.js Outdated Show resolved Hide resolved
}

const targetFeature = features[0];
if (this.props.tooltipState) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this check. Should we update the tooltip lat/lon as the user continues to move inside of large features?

Copy link
Contributor Author

@thomasneirynck thomasneirynck Mar 6, 2019

Choose a reason for hiding this comment

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

I wouldn't. It's not doing that now either.

It's getting to video-game-ish when it does move with pointer imho. When we do the on-click handling, we might want to put the tooltip at the place of click/center of feature,...

x-pack/plugins/maps/public/components/map/mb/view.js Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/components/map/mb/view.js Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/components/map/mb/view.js Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/store/map.js Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/store/map.js Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm
code review, tested in chrome

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@thomasneirynck thomasneirynck merged commit 6b6e670 into elastic:master Mar 7, 2019
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Mar 7, 2019
This is an internal refactor:
- move tooltip management out of layers, and to the mapbox-component
- use global handler iso of multiple handlers on individual layers (this did remove the cursor-pointer change, since we no longer are explicitly handling on-enter/leave events).
- put tooltip state in store

Fixes bugs:
- when layer is removed, any corresponding tooltip should be removed as well
- when layer is made invisible, any corresponding tooltip should be removed as well
PhilippBaranovskiy pushed a commit to PhilippBaranovskiy/kibana that referenced this pull request Mar 7, 2019
This is an internal refactor:
- move tooltip management out of layers, and to the mapbox-component
- use global handler iso of multiple handlers on individual layers (this did remove the cursor-pointer change, since we no longer are explicitly handling on-enter/leave events).
- put tooltip state in store

Fixes bugs:
- when layer is removed, any corresponding tooltip should be removed as well
- when layer is made invisible, any corresponding tooltip should be removed as well
thomasneirynck added a commit that referenced this pull request Mar 7, 2019
This is an internal refactor:
- move tooltip management out of layers, and to the mapbox-component
- use global handler iso of multiple handlers on individual layers (this did remove the cursor-pointer change, since we no longer are explicitly handling on-enter/leave events).
- put tooltip state in store

Fixes bugs:
- when layer is removed, any corresponding tooltip should be removed as well
- when layer is made invisible, any corresponding tooltip should be removed as well
@thomasneirynck thomasneirynck mentioned this pull request Mar 8, 2019
6 tasks
@nreese nreese added the release_note:skip Skip the PR/issue when compiling release notes label May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:skip Skip the PR/issue when compiling release notes v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants