-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
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
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,8 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) { | |
| return Lib.coerce(layoutIn, layoutOut, layoutAttributes, attr, dflt); | ||
| } | ||
|
|
||
| coerce('clickmode'); | ||
|
||
|
|
||
| var dragMode = coerce('dragmode'); | ||
| if(dragMode === 'select') coerce('selectdirection'); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -174,6 +174,7 @@ polygon.tester = function tester(ptsIn) { | |||||
| }; | ||||||
| }; | ||||||
|
|
||||||
| // TODO Somewhat redundant to multiTester in 'lib/select.js' | ||||||
|
||||||
| if(Array.isArray(ptsIn[0][0])) return polygon.multitester(ptsIn); |
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 not multitester. You're correct, multitester is currently used in tester in lib/polygon.js.
Now, if I recall correctly, that polygon.multitester call in polygon.tester was only intended for mulit-selections. Git blame points us to b800e55 which is consistent.
When commenting out this line
Line 34 in cbd9f07
| if(Array.isArray(ptsIn[0][0])) return polygon.multitester(ptsIn); |
no test fail.
So yeah, I think it's safe to 🔪 everything about multitester in lib/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".
Uh oh!
There was an error while loading. Please reload this page.