search

teodosii / react-notifications-component

Delightful and highly customisable React Component to notify your users
https://teodosii.github.io/react-notifications-component/
MIT License
1.27k stars 73 forks source link

Replace existing notifications w/ same id #99

Closed kkkrist closed 2 years ago

kkkrist commented 3 years ago

… rather than adding a copy

Resolves #90

teodosii commented 3 years ago

Does this take height adjustment into consideration? If the initial notification was 30px and the new content spans to 90px then the notification display is messed up.

kkkrist commented 3 years ago

Does this take height adjustment into consideration? If the initial notification was 30px and the new content spans to 90px then the notification display is messed up.

Thank you for your reply! First of all, I'm really sorry for the sloppy PR. Missing height adjustment wasn't the only problem with it :-( I've taken more time to get familiar with the code base today and (hopefully) fixed it: The setState() statement now works as intended, plus the notification height is being re-calculated.

Since the ids are now more essential than before, I also went ahead and made the parseNotification() helper use (somewhat) random default ids rather than the counter value. I hope this prevents collisions in case ids from a database with sequential numbers are used.

Here's a video of my testing:

https://user-images.githubusercontent.com/3873068/112510199-80772b80-8d91-11eb-998d-eda73d11eb80.mp4

UlbeFerranCastillo commented 3 years ago

Hi! You could test this pull request and if the result is positive, merge it with master. It is quite useful that notifications with the same id are replaced, since otherwise there are alerts for duplicate Id and it is annoying to see the same message several times, especially when you have different asynchronous calls and the server returns a 403 there are as many notifications as errors . Obviously I have my way of controlling that it does not happen, but it is not a natural behavior and I think there should be a way to prevent notifications with the same ID from being duplicated.

Thanks !

teodosii commented 2 years ago

Merging this into master, thank you for contributing! Sorry for the extended delay.