Skip to content

Fix x and y interaction modes#6267

Closed
kurkle wants to merge 3 commits intochartjs:masterfrom
kurkle:tooltip-modes-x-and-y
Closed

Fix x and y interaction modes#6267
kurkle wants to merge 3 commits intochartjs:masterfrom
kurkle:tooltip-modes-x-and-y

Conversation

@kurkle
Copy link
Member

@kurkle kurkle commented May 12, 2019

kurkle added 2 commits May 17, 2019 21:39
 - removed redundant Math.abs
 - changed comment wrapping
 - replaced Math.pow with multiplication
@kurkle
Copy link
Member Author

kurkle commented May 17, 2019

Updated based on comments + refactored to remove duplicate code.

@kurkle
Copy link
Member Author

kurkle commented May 19, 2019

Added couple of pens to compare with 'nearest' mode.
After a talk with @simonbrunel I'm not quite sure this should be merged.
Current 'x' and 'y' modes can have valid use cases too.

Maybe we should actually:

  • deprecate x and y
  • add 'band' mode with axis option (= current 'x'/'y' implementation)
  • add useHitradius option to 'nearest' mode (or limitToHitRadius / withinHitradius you name it)
  • update documentation about modes (modes#x, for example, does not describe current behavior)

I think last one in that list is most important :)

Thoughts, @etimberg @nagix @benmccann @simonbrunel ?

@simonbrunel
Copy link
Member

I like the idea of deprecating x and y for a single mode controlled by the axis option, though, I wouldn't call it band but something related to the selection method (e.g. position, coordinates, etc.). I may also deprecate point and nearest and introduce a new nearest: bool option that would apply to all modes.

We would have only 3 modes: position, dataset and index and the deprecated ones (if I'm not wrong):

  • mode: 'x' eq. mode: 'position', axis: 'x'
  • mode: 'y' eq. mode: 'position', axis: 'y'
  • mode: 'point' eq. mode: 'position', axis: 'xy', intersect: true
  • mode: 'nearest' eq. mode: 'position', axis: 'xy', nearest: true

Finally, we could introduce a new limit option and then deprecate the single mode:

  • mode: 'single' eq. mode: 'position', axis: 'xy', intersect: true, limit: 1

Actually, we may prefer to use limit: 1 instead of introducing a nearest: true option, but it wouldn't work the same for point at the exact same distance (nearest returning all the points in this case).

@kurkle
Copy link
Member Author

kurkle commented May 20, 2019

In addition to what @simonbrunel suggested, how about a group (or count) option with 'dataset', 'index' and 'distance' as values?
limit would apply to number of "groups".

old to new:

  • mode: 'single' - mode: 'position', axis: 'xy', intersect: true, limit 1
  • mode: 'index' - mode: 'position', group: 'index', limit 1
  • mode: 'dataset' - mode: 'position', group: 'dataset', limit 1
  • mode: 'point' - mode: 'position', axis: 'xy', intersect: true
  • mode: 'nearest' - mode: 'position', group: 'distance', limit 1
  • mode: 'x' - mode: 'position', axis: 'x'
  • mode: 'y' - mode: 'position', axis: 'y'

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

The current implementation is IMO a breaking change that we shouldn't merge.

@benmccann
Copy link
Contributor

What's the breaking change in the current PR?

@nagix
Copy link
Contributor

nagix commented May 22, 2019

This is similar to @kurkle's proposal, but uses mode to form groups and limit to apply to the number of groups. If mode: all, limit: 1, only 1 item at the top on the list sorted by distance will be returned.

  • mode: 'single' - mode: 'all', axis: 'xy', intersect: true, limit: 1
  • mode: 'index' - mode: 'index', limit: 1
  • mode: 'dataset' - mode: 'dataset', limit: 1
  • mode: 'point' - mode: 'all', axis: 'xy', intersect: true
  • mode: 'nearest' - mode: 'position', limit: 1
  • mode: 'x' - mode: 'all', axis: 'x', intersect: true
  • mode: 'y' - mode: 'all', axis: 'y', intersect: true

@nagix
Copy link
Contributor

nagix commented May 22, 2019

Below might be even simpler - using mode: 'intersect' to force intersect: true and limit to apply the number of items in a group (assuming items are sorted by distance in a group).

  • mode: 'single' - mode: 'intersect', axis: 'xy', limit: 1
  • mode: 'index' - mode: 'index'
  • mode: 'dataset' - mode: 'dataset'
  • mode: 'point' - mode: 'intersect', axis: 'xy'
  • mode: 'nearest' - mode: 'position'
  • mode: 'x' - mode: 'intersect', axis: 'x'
  • mode: 'y' - mode: 'intersect', axis: 'y'

@KimGustafsson
Copy link

What's the status on this? Will there be any mode x with only one item per dataset?

@kurkle
Copy link
Member Author

kurkle commented Jan 8, 2020

This is not currently interesting to me, 'nearest' mode works well enough in my use cases.

@kurkle kurkle closed this Jan 8, 2020
@kurkle kurkle deleted the tooltip-modes-x-and-y branch February 19, 2020 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants