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

Add multiselect #2140

Merged
merged 9 commits into from Nov 13, 2017

Conversation

Projects
None yet
6 participants
@dy
Member

dy commented Nov 2, 2017

Reproduces #1853

@etpinard

This comment has been minimized.

Show comment
Hide comment
@etpinard

etpinard Nov 2, 2017

Member

Yep, npm run test-jasmine -- --bundleTest=ie9_test.js correctly fails as poly-bool has a few Unit8Array calls:

image

Now, I don't think multi-selections in IE9 is a must. Maybe there's a way to make poly-bool not invoke Unit8Array on load?

Member

etpinard commented Nov 2, 2017

Yep, npm run test-jasmine -- --bundleTest=ie9_test.js correctly fails as poly-bool has a few Unit8Array calls:

image

Now, I don't think multi-selections in IE9 is a must. Maybe there's a way to make poly-bool not invoke Unit8Array on load?

@dy

This comment has been minimized.

Show comment
Hide comment
@dy

dy Nov 2, 2017

Member

@etpinard turns out poly-bool is a huge dependency with redundancy. Replaced it with polybooljs - faster and smaller alternative

Member

dy commented Nov 2, 2017

@etpinard turns out poly-bool is a huge dependency with redundancy. Replaced it with polybooljs - faster and smaller alternative

var modes = ['lasso', 'select'];
return modes.indexOf(dragmode) !== -1;
return dragmode === 'lasso' || dragmode === 'select';

This comment has been minimized.

@etpinard

etpinard Nov 2, 2017

Member

Way better 🐎 . Thanks!

@etpinard

etpinard Nov 2, 2017

Member

Way better 🐎 . Thanks!

@chriddyp

This comment has been minimized.

Show comment
Hide comment
@chriddyp

chriddyp Nov 3, 2017

Member

This is so awesome.
multiple-selections

Member

chriddyp commented Nov 3, 2017

This is so awesome.
multiple-selections

@etpinard

This comment has been minimized.

Show comment
Hide comment
@etpinard

etpinard Nov 3, 2017

Member

But I'd wait for persistent selections PR merged first

Sounds good @dfcreative Looks like everyone is happy with e8c18f0 🎉

Now, would you mind adding a few test cases in https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/select_test.js to 🔒 down the behavior?

Member

etpinard commented Nov 3, 2017

But I'd wait for persistent selections PR merged first

Sounds good @dfcreative Looks like everyone is happy with e8c18f0 🎉

Now, would you mind adding a few test cases in https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/select_test.js to 🔒 down the behavior?

@etpinard etpinard added this to the v1.32.0 milestone Nov 3, 2017

@etpinard

@dfcreative thanks for adding those test cases! This PR is looking great.

I made a few comments, the most important being the moving most of the new cartesian/dragbox.js logic to cartesian/select.js so that all subplot types can support multi-select.

Show outdated Hide outdated src/plots/cartesian/dragbox.js
Show outdated Hide outdated src/plots/cartesian/dragbox.js
Show outdated Hide outdated src/plots/cartesian/dragbox.js
})
.then(function() {
// sub selection
drag([[219, 143], [219, 183]], {altKey: true});

This comment has been minimized.

@etpinard

etpinard Nov 13, 2017

Member

Brilliant tests

Thanks

@etpinard

etpinard Nov 13, 2017

Member

Brilliant tests

Thanks

This comment has been minimized.

@dy

dy Nov 13, 2017

Member

:) you're welcome!

@dy

dy Nov 13, 2017

Member

:) you're welcome!

@etpinard

This comment has been minimized.

Show comment
Hide comment
@etpinard

etpinard Nov 13, 2017

Member

Beautifully done @dfcreative, merge away 💃

Member

etpinard commented Nov 13, 2017

Beautifully done @dfcreative, merge away 💃

@dy dy merged commit 2656167 into master Nov 13, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@dy dy deleted the multiselect branch Nov 13, 2017

@PavelGolodoniuc

This comment has been minimized.

Show comment
Hide comment
@PavelGolodoniuc

PavelGolodoniuc Mar 2, 2018

Is there a way to disable multiselect mode and return to the old behaviour when Shift was invoking pan mode?

PavelGolodoniuc commented Mar 2, 2018

Is there a way to disable multiselect mode and return to the old behaviour when Shift was invoking pan mode?

@dy

This comment has been minimized.

Show comment
Hide comment
@dy

dy Mar 2, 2018

Member

@PavelGolodoniuc pan mode can be enabled by Ctrl key, is that a case?

Member

dy commented Mar 2, 2018

@PavelGolodoniuc pan mode can be enabled by Ctrl key, is that a case?

@PavelGolodoniuc

This comment has been minimized.

Show comment
Hide comment
@PavelGolodoniuc

PavelGolodoniuc Mar 6, 2018

@dfcreative it doesn't seem to be the case. When "Box select" is mode is selected Ctrl key doesn't seem to enable pan. Also our application traditionally used Ctrl key over the plot for a user-defined functionality.

PavelGolodoniuc commented Mar 6, 2018

@dfcreative it doesn't seem to be the case. When "Box select" is mode is selected Ctrl key doesn't seem to enable pan. Also our application traditionally used Ctrl key over the plot for a user-defined functionality.

@PavelGolodoniuc

This comment has been minimized.

Show comment
Hide comment
@PavelGolodoniuc

PavelGolodoniuc Mar 12, 2018

@dfcreative, @etpinard Can the multiselect mode including Shift/Alt/Ctrl handler be disabled completely in the Box select mode? In our application we do not allow irregular selections in general and thus would like to disable this functionality completely.

PavelGolodoniuc commented Mar 12, 2018

@dfcreative, @etpinard Can the multiselect mode including Shift/Alt/Ctrl handler be disabled completely in the Box select mode? In our application we do not allow irregular selections in general and thus would like to disable this functionality completely.

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