Skip to content

Support select by nearest point to mouse (v2)#1064

Open
squaregoldfish wants to merge 10 commits intodanvk:masterfrom
quince-science:master
Open

Support select by nearest point to mouse (v2)#1064
squaregoldfish wants to merge 10 commits intodanvk:masterfrom
quince-science:master

Conversation

@squaregoldfish
Copy link
Contributor

NOTE This Pull Request supersedes PR #917. Since that PR is now nearly 8 years old, it was a long way behind the current master branch. Surprisingly it needed barely any changes to bring it up to date. Below is the description copied from #917.


This pull request implements a new selectMode option that allows switching the point selection mode between the closest X axis value (current behaviour) and the closest point to the mouse.

The option is activated by setting selectMode to either closest-x (the default current behaviour) or euclidian.

The changes that locked series, highlighted series and stacked graphs make to the selection behaviour have been carried across to the new euclidian mode to mimic existing behaviour as closely as possible. Tests have been added for all combinations of these options.

This work is related to issue #371. An additional change to getSelection() is required to handle multiple points with the same X axis value (#914), which is also included here.

There are two open questions:

  1. I have put the documentation for selectMode under Interactive Elements. If you want it in another section, let me know.
  2. I'm using findStackedPoint as it is currently written. However we now only use the series name, and none of the other information it returns. Should I cut down the method to only return a series name (and rename it to findStackedSeries), or leave it as it is in case it's useful in the future?

@mirabilos
Copy link
Collaborator

Thanks, I appreciate the effort, I’ll look into this when time permits (unfortunately, I’ve changed employers so this is all spare-time now, which is split over so many FOSS projects…).

I’m a bit wary that you added a dependency though. What is that one for? (I’m building this inside of Debian normally, and I’d even want to reduce it to ES5 to get rid of Babel eventually, so I wouldn’t want to add anything new as dependency. Test dependencies, with the tests run in an npm environment, are usually no problem, given they need things like PhantomJS already anyway.)

@mirabilos
Copy link
Collaborator

Just did have a short look at findStackedPoint.

It is marked as private… so simplifying it to return only what’s needed is probably okay, and perhaps sensible anyway to reduce amount of instructions run, if meaningfully doable.

I don’t quite see how it could be shrunk, other than removing the assignment to closestPoint and simplify the return from Object to name. In that case, keeping it as-is to avoid breaking something that does rely on it might be equally wise. Or do you have some optimisation in mind that makes it substantially simpler?


Documentation section: hm. I find them not very clear anyway. It’s probably fine, did not look at it in detail yet. (It’s high pollen season.)

@squaregoldfish
Copy link
Contributor Author

Re the dependency, it's 100% down to my unfamiliarity with node. After updating to the latest master, ‎auto_tests/tests/select_mode.js wasn't compiling, and a bit of searching got me to replace es6 with es in the import. At that point the node builder added the new dependency. I'm sure it's not needed, but right now I don't know enough to fix it. I'll see if I can find some time to look at it, but I can't guarantee I'll do the right thing...

For findStackedPoint I see no compelling reason to change it, since it works as it is. As you say, it might still come in handy in the future so we might as well leave it.

@squaregoldfish
Copy link
Contributor Author

It turns out that the promise dependency wasn't needed at all, so I've taken it out.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants