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

InViewportDirective should emit when the host element is destroyed #1418

Closed
tinesoft opened this issue Nov 14, 2023 · 6 comments · Fixed by #1425
Closed

InViewportDirective should emit when the host element is destroyed #1418

tinesoft opened this issue Nov 14, 2023 · 6 comments · Fixed by #1425

Comments

@tinesoft
Copy link

Hi,

First of all, thanks for this useful library.

We came across an edge case with inViewportAction event from the inViewportDirective : It will NOT be emitted when the hosting component is destroyed.... thus failing to notify the "listeners" of that event, that the element is no longer visible, because destroyed/removed from the DOM...

We think, this line:

this.emitAction(undefined, false);

should be called, after this line:

this.inViewportService.unregister(this.nativeElement, this.config);

I can provide a PR with the fix if you agree

@k3nsei
Copy link
Owner

k3nsei commented Nov 14, 2023

Happy to hear nice words.

Could you provide example scenario in https://stackblitz.com/ ??

@tinesoft
Copy link
Author

tinesoft commented Nov 14, 2023

Happy to hear nice words.

Sure! well deserved!

Could you provide example scenario in stackblitz.com ??

https://ng-in-viewport-gb-ex-1-vsec6f.stackblitz.io

It was derived from the one from the docs.
Open the console to see the expected behavior
I use the "Remove Item" button simulate an item being destroyed.
I would expect to see "target: 'Item X' is visible: false" in the console, when removing the item...

k3nsei added a commit that referenced this issue Nov 14, 2023
chore: extend peer deps to include angular v17

Refs: #1418
Release-As: 16.1.0
@k3nsei
Copy link
Owner

k3nsei commented Nov 14, 2023

@tinesoft could you confirm that proposed solution in #1419 is what you are expecting?

@k3nsei
Copy link
Owner

k3nsei commented Nov 14, 2023

@all-contributors
please add @tinesoft for ideas
please add @tinesoft for review

Copy link
Contributor

@k3nsei

I've put up a pull request to add @tinesoft! 🎉

k3nsei added a commit that referenced this issue Nov 14, 2023
chore: extend peer deps to include angular v17

Refs: #1418
Release-As: 16.1.0
k3nsei added a commit that referenced this issue Nov 14, 2023
chore: extend peer deps to include angular v17

Refs: #1418
Release-As: 16.1.0
k3nsei added a commit that referenced this issue Nov 14, 2023
chore: extend peer deps to include angular v17

Refs: #1418
Release-As: 16.1.0
k3nsei added a commit that referenced this issue Nov 14, 2023
chore: extend peer deps to include angular v17

Refs: #1418
Release-As: 16.1.0
@tinesoft
Copy link
Author

tinesoft commented Nov 14, 2023

Thanks for the very fast ⚡ reply/action @k3nsei !

As a fellow OSS maintainer , I appreciate and value this ^^

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 a pull request may close this issue.

2 participants