feat(lib): use inject to pass container reference to selectableItem #112

bvkimball · 2020-11-06T03:15:15Z

Needed to use [dtsSelectItem] in grandchildren components which can't be found @ContentChildren

Now using dependency injection i pass the reference of the SelectContainerComponent to any SelectItemDirective that is a child of that container regardless of how deep in the tree the component is.

This is similar to how material does this for drag and drop components for the CDK. see here

Just pushing this out to see if there is interest in adopting this change, currently the demo app work as is, but I can update the demo app with an example of nested components using the directive and do a little more cleanup.

clean up the code
make sure all tests pass
maybe create an additional example section to show this in action
update the readme accordingly
write tests for this change (if needed)

ps. awesome library

d3lm · 2020-11-06T12:46:15Z

Hey @bvkimball, thanks for your PR. This is actually something I was fiddling with as well at some point but it never made it into master. I like this refactoring and agree with what you outlined above. I am definitely down to adopting this PR if you:

clean up the code
make sure all tests pass
maybe create an additional example section to show this in action
update the readme accordingly
write tests for this change (if needed)

How does that sound?

Thanks for your contribution 👍 and I am glad to hear you like this library and it's useful to you.

bvkimball · 2020-11-06T14:07:43Z

Awesome! I will try to get this cleaned up over the weekend.

d3lm · 2020-11-06T17:56:37Z

Love it. Thanks a bunch!

refactor: removed dead code from change and fixed circular dependency caused by DI

bvkimball · 2020-11-08T03:06:35Z

Not perfect yet but I think tests are working again. Issue caused because the Parent-Child Injection causes circular dependency.

The demo is a bit contrived but it is similar to the usecase I am using the library for. I intend to use it with the CDK DragAndDropModule, so that what the demo does. Here is a gif of the demo.

When I re-order the tasks then try to use the select box it selects the task based on the previous position, i tried running selectionContainer.update() but that didn't fix the issue. I am trying to make sure this isn't something i caused with the change or the demo code itself.

Another issue I am looking at is that click to select doesn't seem to work, i think that might be a conflict with Material or the Drag&Drop Module too.

d3lm · 2020-11-08T10:34:06Z

Yep that demo is looking slick! Thanks for this. Will be a great addition. Make sure to not break existing functionality. As to the "click to select", I am not sure. Could be indeed a conflict with the Drag & Drop module.

d3lm · 2020-11-08T10:34:57Z

Let me know when this PR is ready for review and I ll look at it. Maybe you can also take the list of action items I posted earlier and make a small todo list, so we keep track of what's done.

test(app): added cypress test for new example demo

bvkimball · 2020-11-09T15:16:17Z

I think everything is done and working, and I add docs and some cypress test.
The issues with select are caused by the Angular CDK DragDrop swallows all mouse events.
Let me know about any changes or if you want me to rename any of the methods or tokens.

I believe there are no breaking changes unless someone was using $selectItems which is the QueryList that was removed. that was public but really should have been used for Internal use only....

d3lm · 2020-11-10T10:00:31Z

Yep, agreed that the QueryList shouldn't have been public in the first place. If it causes a breaking change we can revert this and publish this as a new major version.

I ll look at your changes today and review everything.

d3lm

It's already looking really good. I love this addition / change.

Left a few remarks though.

Also, can you change dragndrop to be drag-and-drop?

Thanks for this contribution!

README.md

d3lm · 2020-11-10T10:26:25Z

projects/ngx-drag-to-select/src/lib/tokens.ts

@@ -1,5 +1,7 @@
+import { ComponentType } from '@angular/cdk/portal';


Let's not use this, because the CDK is not a peer dependency of this library and IMO we shouldn't use anything from the CDK for the library.

agreed this was silly of me, I just created a version of ComponentType in the models.ts, it is just a Util type anyway that wraps a interface in a constructable, hope this works.

src/app/dragndrop/dragndrop.component.html

src/app/dragndrop/task-list/task-list.component.ts

projects/ngx-drag-to-select/src/lib/select-container.component.ts

docs: updated grammar

bvkimball · 2020-11-11T00:24:59Z

I think i have updated everything correctly, sorry about my poor grammar skills ;)

d3lm

Left a few minor remarks, most of which were comments from my previous PR which have not been updated. Not sure if the PR was not entirely updated or if you just forgot a few comments. Not a biggy tho.

README.md

d3lm · 2020-11-11T08:10:03Z

cypress/integration/drag-and-drop.spec.ts

+    it('should drag to new list', () => {
+      getDragAndDropExample().within(() => {
+        getTodoList()
+          // Select First 3 items in list


Did you forget this?

d3lm · 2020-11-11T08:10:09Z

cypress/integration/drag-and-drop.spec.ts

+          .getSelectItem(2)
+          .dispatch('mousemove')
+          .dispatch('mouseup')
+          // Click on second item


And here too.

d3lm · 2020-11-11T08:10:15Z

cypress/integration/drag-and-drop.spec.ts

+          // Click on second item
+          .getSelectItem(1)
+          .dispatch('mousedown', { button: 0 })
+          // Drag to Select Item in other list


Same again.

projects/ngx-drag-to-select/src/lib/select-container.component.ts

style: update whitespace

bvkimball · 2020-11-11T14:04:46Z

ok, sorry about those missed comments, I wish GitHub had an easy way to view them in a list. I resolved the old outdated comments.

d3lm

This looks good now! Thanks a lot for this PR. I think it's a great addition.

d3lm

Forgot to mention that I'd like to move the drag & drop demo above the mobile demo.

Also, I found some more cosmetic issues. I know, that sucks but I cannot really automatically enforce newlines in scss files.

Can you again explain why range selection and CMD click doesn't work anymore with the drag and drop demo? Is that because Angular Material intercepts the click?

src/app/drag-and-drop/task/task.component.scss

src/app/drag-and-drop/task-list/task-list.component.scss

src/app/app.module.ts

style(app): linting some scss

bvkimball · 2020-11-12T15:55:37Z

No worries, updated. I also added the *ngIf="isDesktop" to hide the drag-and-drop demo on mobile.

Yeah the click to select functionality doesn't work because of the DragDropModule the cdkDrag directive captures all mouse events. There are PRs, bugs, and feature requests for it as seen here: angular/components#19674

It would be awesome if the DragDropModule didn't do this because then the 2 libraries pair together very well.

bvkimball · 2020-11-12T16:08:50Z

I guess a solution could be to pass mouse-events from the selectItem as a "CustomEvent" or pass through to the parent via the reference we just added via DI.

d3lm · 2020-11-12T16:30:06Z

Yea ok, that makes sense. Then I guess we can ignore this for now. It's fine because it's mostly related to the CDK then.

Tests are failing now. I think you have to update your tests to account for the desktop / mobile change.

bvkimball · 2020-11-12T16:55:31Z

Yeah, Cypress error was because the "Item" animates while you drag to reorder, and because the demo isn't at the bottom of the screen anymore the scrollIntoView causes some issue with the "drop location". I changed the test to drag items "down" instead of up. Tested manually too, the issue is just nuances with faking the events for testing.

d3lm · 2020-11-12T21:30:21Z

So when I try the drag and drop I have this weird behavior when you move an item in the same list, then something's off with the placeholder. Would you mind checking this? Seems to be wrong behavior. Any idea why that is?

bvkimball · 2020-11-13T15:25:22Z

ugh... yeah, i see it, it is caused because i hide app-task which has the dtsSelectItem but the cdkDrag container is is still in the dom... I can fix this sometime today.

d3lm · 2020-11-13T17:19:42Z

Awesome! Thanks. Looking forward to that fix. Then we are good to go I think.

bvkimball · 2020-11-14T02:57:25Z

Ok this fix is not great... Its kindof a hack, but since the cdkDrag really only manipulates the dom based on the element you are dragging and just hide the "other" selected items. then those dom elements are still included in the re-ordering logic, i tried a bunch of things like make them absolute positioned, set height to 0, etc. the only thing that worked was to remove them from the dom and add them back after. Once again this is an issue with the CDKDragModule and more of an issue of trying to do something it was not quite yet built for. I think it is still great to show this still as an example of something a lot of people will want to do with this library.

promoter0100 · 2022-11-02T07:37:35Z

I just come here when trying to use Drag & Drop with this lib, and I wonder if this function works?

feat(lib): use inject to pass container reference to selectableItem

dc37330

docs: added nested directive demo with drag and drop

1f14bd3

refactor: removed dead code from change and fixed circular dependency caused by DI

style: fixed lint error for member ordering

104eeb6

Brian Kimball added 2 commits November 8, 2020 08:06

docs: added new section to README

a7d0a23

docs(lib): added section to readme about components capturing events

70e97a1

test(app): added cypress test for new example demo

fix(lib): new Token was not export with the public api

2b28e38

d3lm requested changes Nov 10, 2020

View reviewed changes

projects/ngx-drag-to-select/src/lib/select-container.component.ts Outdated Show resolved Hide resolved

Brian Kimball added 2 commits November 10, 2020 19:14

refactor(app): renamed drag-and-drop

1e70169

docs: updated grammar

refactor(app): forgot to add new files

464791c

d3lm requested changes Nov 11, 2020

View reviewed changes

style: updated cypress comments

9b13e1a

style: update whitespace

d3lm approved these changes Nov 12, 2020

View reviewed changes

d3lm requested changes Nov 12, 2020

View reviewed changes

refactor(app): moved drag-and-drop above mobile and hides

e98d0b8

style(app): linting some scss

fix(app): fixed cypress test, error with test

5636170

fix(app): visual issues with hidden placeholders

4de8b47

style(app): trying to force tests to re-run

61fda2c

d3lm force-pushed the master branch 3 times, most recently from f737d7d to 1fd7215 Compare December 5, 2021 18:20

PhOder mentioned this pull request Dec 2, 2022

feat(lib): use inject to pass container reference to selectableItem #169

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lib): use inject to pass container reference to selectableItem #112

feat(lib): use inject to pass container reference to selectableItem #112

bvkimball commented Nov 6, 2020 •

edited

Loading

d3lm commented Nov 6, 2020

bvkimball commented Nov 6, 2020

d3lm commented Nov 6, 2020

bvkimball commented Nov 8, 2020

d3lm commented Nov 8, 2020

d3lm commented Nov 8, 2020

bvkimball commented Nov 9, 2020

d3lm commented Nov 10, 2020

d3lm left a comment

d3lm Nov 10, 2020

bvkimball Nov 11, 2020

bvkimball commented Nov 11, 2020

d3lm left a comment

d3lm Nov 11, 2020

d3lm Nov 11, 2020

d3lm Nov 11, 2020

bvkimball commented Nov 11, 2020

d3lm left a comment

d3lm left a comment

bvkimball commented Nov 12, 2020

bvkimball commented Nov 12, 2020

d3lm commented Nov 12, 2020

bvkimball commented Nov 12, 2020 •

edited

Loading

d3lm commented Nov 12, 2020

bvkimball commented Nov 13, 2020

d3lm commented Nov 13, 2020

bvkimball commented Nov 14, 2020

promoter0100 commented Nov 2, 2022

		@@ -1,5 +1,7 @@
		import { ComponentType } from '@angular/cdk/portal';

feat(lib): use inject to pass container reference to selectableItem #112

Are you sure you want to change the base?

feat(lib): use inject to pass container reference to selectableItem #112

Conversation

bvkimball commented Nov 6, 2020 • edited Loading

d3lm commented Nov 6, 2020

bvkimball commented Nov 6, 2020

d3lm commented Nov 6, 2020

bvkimball commented Nov 8, 2020

d3lm commented Nov 8, 2020

d3lm commented Nov 8, 2020

bvkimball commented Nov 9, 2020

d3lm commented Nov 10, 2020

d3lm left a comment

Choose a reason for hiding this comment

d3lm Nov 10, 2020

Choose a reason for hiding this comment

bvkimball Nov 11, 2020

Choose a reason for hiding this comment

bvkimball commented Nov 11, 2020

d3lm left a comment

Choose a reason for hiding this comment

d3lm Nov 11, 2020

Choose a reason for hiding this comment

d3lm Nov 11, 2020

Choose a reason for hiding this comment

d3lm Nov 11, 2020

Choose a reason for hiding this comment

bvkimball commented Nov 11, 2020

d3lm left a comment

Choose a reason for hiding this comment

d3lm left a comment

Choose a reason for hiding this comment

bvkimball commented Nov 12, 2020

bvkimball commented Nov 12, 2020

d3lm commented Nov 12, 2020

bvkimball commented Nov 12, 2020 • edited Loading

d3lm commented Nov 12, 2020

bvkimball commented Nov 13, 2020

d3lm commented Nov 13, 2020

bvkimball commented Nov 14, 2020

promoter0100 commented Nov 2, 2022

bvkimball commented Nov 6, 2020 •

edited

Loading

bvkimball commented Nov 12, 2020 •

edited

Loading