Skip to content
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 to Selection When Using Lasso Tool with Shift Key #851

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

philippfromme
Copy link
Contributor

@nikku
Copy link
Member

nikku commented Jan 22, 2024

Nit-pick: If I start the tool without SHIFT I'd expect the existing selection to go away:

capture 732iKl_optimized

Observation: We break existing SHIFT + drag flow for lasso tool activation, it will always add to existing selection. I'd be fine with that.

Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

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

Let's fix what @nikku pointed out above.

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Jan 25, 2024
@philippfromme
Copy link
Contributor Author

Observation: We break existing SHIFT + drag flow for lasso tool activation, it will always add to existing selection. I'd be fine with that.

I've noticed that and checked how Miro does it. Works the same there.

@philippfromme
Copy link
Contributor Author

Nit-pick: If I start the tool without SHIFT I'd expect the existing selection to go away:

I'm not sure what you mean. What isn't working as expected?

@nikku
Copy link
Member

nikku commented Jan 26, 2024

I'm not sure what you mean. What isn't working as expected?

The moment I start the lasso tool without SHIFT the old selection will vanish (new one will not be added to old one); hence I propose to remove it once the tool is started. Cf. attached screen capture.

@philippfromme
Copy link
Contributor Author

The moment I start the lasso tool without SHIFT the old selection will vanish (new one will not be added to old one); hence I propose to remove it once the tool is started. Cf. attached screen capture.

So you mean you press L and expect the existing selection to go away?

@nikku
Copy link
Member

nikku commented Jan 26, 2024

Yes. Sorry if my explanation was misleading.

@philippfromme
Copy link
Contributor Author

Yes. Sorry if my explanation was misleading.

But that's the same as clicking the palette icon. I wouldn't expect the selection to go away immediately after just activating the tool. I would expect the selection to go away after selecting the recangular area without pressing SHIFT.

@nikku
Copy link
Member

nikku commented Jan 26, 2024

I would expect the selection to go away after selecting the recangular area without pressing SHIFT.

I'd expect the opposite 🙃. Let's do a quick poll to resolve this, and proceed afterwards.

@philippfromme
Copy link
Contributor Author

I'd expect the opposite 🙃. Let's do a quick poll to resolve this, and proceed afterwards.

But that would mean you must press SHIFT while activating the lasso through click or L before even selecting anything.

@nikku
Copy link
Member

nikku commented Jan 26, 2024

To clarify, this is the interaction I'd expect (recorded from excalidraw):

capture J9JaEn_optimized

@philippfromme
Copy link
Contributor Author

I had to keep keepSelection: true; for both #activateLasso and #activateSelection, otherwise the selection is lost even if I just want to hide it during the selection phase. Therefore, the only way of hiding the selection is to use CSS so I added a djs-lasso-selection class.

@nikku
Copy link
Member

nikku commented Jan 29, 2024

I had to keep keepSelection: true; for both #activateLasso and #activateSelection, otherwise the selection is lost even if I just want to hide it during the selection phase. Therefore, the only way of hiding the selection is to use CSS so I added a djs-lasso-selection class.

Why is it lost?

@philippfromme philippfromme marked this pull request as draft January 30, 2024 08:27
@philippfromme
Copy link
Contributor Author

I had to keep keepSelection: true; for both #activateLasso and #activateSelection, otherwise the selection is lost even if I just want to hide it during the selection phase. Therefore, the only way of hiding the selection is to use CSS so I added a djs-lasso-selection class.

Why is it lost?

I've checked again and it's actually restored so there's a different issue. I'll fix that. 😓

@philippfromme
Copy link
Contributor Author

@nikku I found a different solution. Unfortunately, calling Selection#select with add = true on lasso.end isn't possible since the restoring of the selection that Dragging attempts is done only after cleanup has already been fired so there is no way to first let Dragging restore the selection and then add elements to the selection.

@philippfromme philippfromme marked this pull request as ready for review January 30, 2024 09:51
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jan 30, 2024
@philippfromme philippfromme requested a review from barmac January 30, 2024 09:51
@barmac
Copy link
Member

barmac commented Jan 30, 2024

Perhaps Dragging could be even more low-level. It is the selection who should handle selection and not dragging as it is in:

https://github.com/bpmn-io/diagram-js/blob/develop/lib/features/dragging/Dragging.js#L235

@nikku nikku force-pushed the lasso-tool-shift-add branch from f6e83e6 to b6a1912 Compare January 30, 2024 10:49
@barmac barmac force-pushed the lasso-tool-shift-add branch from b6a1912 to c407b8e Compare February 1, 2024 14:12
@barmac
Copy link
Member

barmac commented Feb 2, 2024

After rebase, I cannot run this with bpmn-js due to missing touch support module.

@nikku
Copy link
Member

nikku commented Feb 2, 2024

We'd want to wrap up the touch removal to unblock @philippfromme. I can take that over, or you do it.

@barmac
Copy link
Member

barmac commented Feb 2, 2024

I can look into this on Monday, otherwise feel free to wrap it up.

@nikku
Copy link
Member

nikku commented Feb 2, 2024

@barmac Happy if you take this over 🤞

@barmac
Copy link
Member

barmac commented Feb 5, 2024

Let's ship it, and we can improve on the visuals in the future. Better provide value early and we can also collect feedback on the next steps if necessary.

@barmac barmac merged commit 292894d into develop Feb 5, 2024
12 checks passed
@barmac barmac deleted the lasso-tool-shift-add branch February 5, 2024 14:51
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Feb 5, 2024
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.

3 participants