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

Notifications are never removed from dom #50

Closed
Kepro opened this issue Oct 1, 2017 · 10 comments
Closed

Notifications are never removed from dom #50

Kepro opened this issue Oct 1, 2017 · 10 comments
Assignees
Milestone

Comments

@Kepro
Copy link

Kepro commented Oct 1, 2017

After every item is disappear then we will see in dom react-empty comment, all dead notifications should be removed also internally

@fkhadra
Copy link
Owner

fkhadra commented Oct 1, 2017

The dead notifications are removed. What you are seeing is the toast's containers which are removed after the n+1 iteration. This is done by design.

When you display a notification we check if for the given position a container exists. If yes, use it otherwise create one. In the meantime, we remove all unused container.

Go to the demo page, open your devtools and wait the end of the notification. You will see 6 containers, 1 for each position:
screen shot 2017-10-01 at 20 55 58

Then click on notify to display only one notification. All the containers will be removed except the one used to position the notification:
screen shot 2017-10-01 at 20 58 47

@Kepro
Copy link
Author

Kepro commented Oct 1, 2017

I'm talking about
toast1
toast2

@fkhadra
Copy link
Owner

fkhadra commented Oct 1, 2017

Can you provide a snippet to reproduce it?
Mine is getting empty:
screen shot 2017-10-01 at 21 22 50

@Kepro
Copy link
Author

Kepro commented Oct 2, 2017

@fkhadra I'm using react 15.6.1 are you testing it on 16.0.0?

@fkhadra
Copy link
Owner

fkhadra commented Oct 2, 2017

@Kepro yes I switched to react 16 and the same for the test. You think that it can cause the issue ?

@Kepro
Copy link
Author

Kepro commented Oct 2, 2017

React 16 is handling this in a different way... in react 15.x if you return null then you will see react-empty as I showed in screenshot, but my point was that you still have all elements in your object, so after dismiss you don't remove it from there, this is also wasting memory in application, think if you have some system and it's running 24/7 without reloading and you never remove hidden notifications :)

I don't look at your code, maybe you have internal limit of max notifications and you clearing that queue

@fkhadra
Copy link
Owner

fkhadra commented Oct 2, 2017

@Kepro you're right. I clean the "object" when the container is unmounted.
I got your point, I'll fix this. I guess it will be cleaner. Thanks !

@Kepro
Copy link
Author

Kepro commented Oct 2, 2017

Thanks, sorry for no PR :) but don't have time now...

@fkhadra
Copy link
Owner

fkhadra commented Oct 2, 2017

Don't worry :D. Finding a bug is a way to contribute

@dhirendrarathod2000
Copy link

@fkhadra i checked your demo
it removes from everything HTML dom but it keep container on React Dom

@fkhadra fkhadra added this to the v2.2.0 milestone Oct 11, 2017
fkhadra added a commit that referenced this issue Oct 13, 2017
fkhadra added a commit that referenced this issue Oct 13, 2017
@fkhadra fkhadra closed this as completed Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants