feat(lib): added selectAllUnderCursor config for nested items

d3lm / ngx-drag-to-select

A lightweight, fast, configurable and reactive drag-to-select component for Angular 10 and beyond

https://www.npmjs.com/package/ngx-drag-to-select

MIT License

293 stars 65 forks source link

feat(lib): added selectAllUnderCursor config for nested items #100

Closed sumbricht closed 4 years ago

sumbricht commented 4 years ago

Ok, now I have found some time to play around with the code :-) and I think I have a nice solution approach for #93 that doesn't require any significant changes. Essentially, I have adapted the logic in _onMouseDown to not only check which items' bounding boxes overlap the mouse position, but alternatively use document.elementFromPoint to find the inner-most nested item. This of course needs to be configurable with the previous behavior (treat all matching items as clicked) as the default. For this I have added a boolean input selectAllUnderCursor=true to SelectContainerComponent.

Please have a look and let me know what you think :-)

PS: apologies for the unrelated formatting changes; that was prettier running in your pre-commit hooks ;-), so I can't do much about it

d3lm commented 4 years ago

Thanks for the PR. I ll have a look this week. Can you please also write tests for this? The project uses Cypress, so please also add e2e tests for this new feature and make sure to test edge cases as well. Thanks again for contributing, really appreciate it. From glimpsing at your PR, it looks already good. But I ll give it a full review once you have added some tests.

d3lm commented 4 years ago

I also saw that the commit message contains past tense. Can you change your commit message to present tense and change the message to describe the overall feature and not just the config option. I suggest something along the lines of feat(lib): add feature to allow selecting nested children.

sumbricht commented 4 years ago

Hi @d3lm, I have started writing tests but it's taking a bit more time due to two things:

I have to add nested children to some of the "Document" tiles and change the tiles slightly in order to accommodate for the child elements
The edge case of range selection using the Shift key is a bit annoying as it's currently behaving inconsistently. When selecting 3 adjacent Document tiles, the children of the first (starting) and seconds (interim) tile are selected as well, but the children of the third (ending) tile aren't. I'm not sure if I can also fix this while I'm at it, but strictly speaking it's more of a problem of having nested elements and not the "selectAllUnderCursor" functionality.

What do you think how we should proceed for the second point?

d3lm commented 4 years ago

I suggest that we have another section under the "Desktop Demo" for the use case of nested children so we don't modify the original and make it too complex. It's already getting a bit complex with all those flags. So let's just create a dedicated sub-section for this use case.

I also vote for fixing the range selection. IMO the intended behavior for nested children is that when selecting a range, that all sub-children are selected as well. This could maybe be disabled / enabled with another option.

The option is not necessary for this PR to be merged but the range selection should not behave inconsistently or break when introducing a new feature such as nested children. A new feature should not break existing features.

Does that make sense? Any ideas?

sumbricht commented 4 years ago

Fully agree, I have started creating a separate section between "Desktop Demo" an "Mobile Demo" for the demo this use case.

The more I think about this and try to implement, the trickier it's getting though X-). My current feeling is that range selection is only meaningful among sibling elements on the same nesting level. Otherwise, it's quite arbitrary how a range selection across nesting levels should be handled. Then it would make sense to have another flag to specify whether children of the selected elements should be marked as selected as well or not.

What do you think?

d3lm commented 4 years ago

I totally agree that this can easily become very tricky. However, why does the range selection get arbitrary? I mean when you start a range selection and select one item that has children it will first select its children in order and when all have been selected, we can select the next top level item. However, I think this should definitely be an option and it's not super duper straight forward to implement. This can become complicated quite easily.

What issues are you facing exactly? Maybe we can find a solution together and discuss them.

Satish-TD commented 4 years ago

Why range selection is not happening with new code using shift + click

d3lm commented 4 years ago

@Satish-TD If you feel that this is an issue please create a new issue report and provide a reproducable using StackBlitz. This thread is a PR and not the right place for reporting issues, because this can easily get lost in the discussion here and to not forget about issues, there are the issues. I am more than happy to take a look if you create a separate issue for the behavior you're experiencing and provide a demo that shows the issue.