Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
1852 persistent click via select points #2944
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
1852 persistent click via select points #2944
Changes from 47 commits
2fbaaaa
346bad9
4f8dd92
6a87769
2d59365
5a13573
d3b4a0f
36145df
73e5085
1a89040
a6e49fd
bb44b6d
48a0a36
4639718
497747b
744ab56
7da973b
ee5f990
6ac1a08
e11faf5
bef47a3
c394a8b
5b08a2c
e763964
41e3ba5
6a2ae3b
e7c2240
23024b2
cbd9f07
7eba640
92ee0fa
3073f61
3767ce2
8a4fc99
db3e126
b3984f7
2f7ad27
d7c1434
e895efd
60317b1
61b1a32
e460e46
2ef3fce
ac4b909
05b50b9
88e9993
f247d93
90ba209
2bc582b
96fcdd7
a5a90f9
87d0497
30789dd
cc2c9ba
32aa219
b6bd813
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still missing @alexcjohnson's smart default suggestion where when a user sets
clickmode: 'select'
the defaulthovermode
value becomes'closest'
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, I misunderstood that. I thought mentioning in
clickmode
's description that hovermode set to closest is best for most applications was "enough".So what we want to have is when
clickmode
includes theselect
flag and nothing is set explicitly forhovermode
, the default ofhovermode
should beclosest
. Is that right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! Which should be as simple as modifying the
hovermodeDflt
logic below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think
polygon.multiTester
gets called anywhere now. Would you mind 🔪 ing it?This makes perfect sense. Using a polygon multi-tester function for click-to-select would have been confusing 👌
That said, I don't think we need another file in
lib/
for this thing. Plus, having files calledselect.js
inplots/cartesian
, all overtraces/
and nowlib/select.js
probably isn't ideal. Would it be ok to just move the contents of your newlib/select.js
tocartesian/select.js
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not just used by cartesian though, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. E.g. it is used inpolar
andchoropleth
.Update: Sorry, I was referring to
polygon.tester
.cartesian/select.js
is in fact the only module requiringlib/select.js
. So I think it'd be better to move it there. Do you agree, @alexcjohnson ?Nope. It's called in
polygon.tester
in the first line.Update:
polygon.tester
is used bypolar
,choropleth
etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, all of
cartesian/select.js
should be moved out ofcartesian/
- as @alexcjohnson said it is not just used for cartesian subplots anymore.What I was suggesting is to move the contents of
lib/select.js
into the same file ascartesian/select.js
as the contents oflib/select.js
aren't used anywhere else.Once that's done, we can move
cartesian/select.js
to eithercomponents/fx
(side-by-side with the hover routines) or in a newcomponents/
directory e.g.components/select
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Moved contents of
lib/select.js
into cartesian/select.js`.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rmoestl thanks for moving the contents to
cartesian/select.js
🚚Now, would you mind 🔪 the
multiTester
routine and theTODO
above from this file.polygon.multiTester
isn't used anywhere now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, to repeat that, but isnt't
polygon.multiTester
called hereplotly.js/src/lib/polygon.js
Line 34 in cbd9f07
Or can we 🔪 that line as well? I hesitated to do so back then because
polygon.tester
is called from multiple places other than selection code - potentially with a parameter type that requires this very line.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad! I grep'ed for
multiTester
notmultitester
. You're correct,multitester
is currently used intester
inlib/polygon.js
.Now, if I recall correctly, that
polygon.multitester
call inpolygon.tester
was only intended for mulit-selections. Git blame points us to b800e55 which is consistent.When commenting out this line
plotly.js/src/lib/polygon.js
Line 34 in cbd9f07
no test fail.
So yeah, I think it's safe to 🔪 everything about
multitester
inlib/polygon.js
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think we have enough tests that you could try replacing that line with
if(Array.isArray(ptsIn[0][0])) throw new Error('multitester called!');
or something and see if it ever happens.
But if it does, could that line just point to the new
multiTester
? Of course doing that would make a circular reference unless these are brought into the same file - which might be better anyway, I feel like these belong together (even if we can't quite agree on where that together should be...)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running all tests didn't throw the error. I left throwing the error in place and added a nice comment to it, so that future readers understand the "why".