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

Feature/drag elements as a block #162

Merged

Conversation

rendertom
Copy link
Contributor

User story:
As a user I do not want draggable elements to get cramped into one blob when I hit edges of the container while dragging.

Description:
Dragging multiple elements and hitting the edges of the container does not stop the drag event. Elements, that hit the edge become non-draggable (as expected), however, the rest of the elements are still being dragged. Therefore, when hitting all of the container edges, all elements might get cramped into a blob in DragSelect v2.5.5.

This PR adds an option to prevent this effect. Once implemented, the drag event will stop (based on drag direction) if at least one of the draggable elements hits the edge.

Settings.dragAsBlock: boolean, defaults to false: Whether to drag multiple elements as a single block (true) or as individual items (false).

Usage example:

const ds = new DragSelect({
  selectables: document.getElementsByClassName('item'),
  area: document.getElementById('container'),
  dragAsBlock: true, // optional, defaults to false
});

@ThibaultJanBeyer
Copy link
Owner

Hi @rendertom thanks a lot for your contribution! 🎉 🎉 🎉
This sounds great, sounds like this should be the default behaviour? I’ll have a look asap 👀

@ThibaultJanBeyer
Copy link
Owner

ThibaultJanBeyer commented Feb 1, 2023

Just tested it locally and seems to work great. I think in the long run this should probably be default behaviour, what do you think?
Also, would you mind adding a test case file for this change? This also helps reviewers like me play around and test various use-cases! Thank you 🤗


elements = Array.isArray(elements) ? elements : [elements]
elements.forEach((element) => {
const elementRect = element.getBoundingClientRect()
Copy link
Owner

Choose a reason for hiding this comment

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

the only issue I see here is that re-calculating the rects is quite expensive. The rect is also calculated within the moveElement method on every move. Would it make sense to writte this somehow that the element rect is only calculated once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

I pre-calculated the selection rect on drag start, so it should improve the performance. I also believe that we could pre-calculate the react of each element on dragStart as well, and then pass that value to moveElement method. However, I am not sure if such change belongs on this PR though.

@rendertom
Copy link
Contributor Author

Thanks @ThibaultJanBeyer for kind reply. I am happy to contribute.

I am not familiar with puppeteer and jest tests, so I am not sure if I can be of any help here. Regarding the default behaviour - personally for me, I think I'd chose it as a default, as it is more expected. But that's just me. Feel free to change functionality how you feel is best.

@ThibaultJanBeyer ThibaultJanBeyer merged commit bb0bd8e into ThibaultJanBeyer:master Feb 26, 2023
@ThibaultJanBeyer
Copy link
Owner

I'm working on a few improvements on this one to potentially make it the standard behaviour. Then it will come with the next version.

@ThibaultJanBeyer
Copy link
Owner

ThibaultJanBeyer commented Mar 5, 2023

So I found a few issues when running the tests which I was not able to fix in a few hours. I want to fix them first before making this a default behaviour. So this is available now in 2.7.0 behind the flag dragAsBlock as you intended until I manage to fix the issues.

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.

None yet

2 participants