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

Unnecessary rerender/readding for existing messages #17

Closed
LKay opened this issue Nov 15, 2016 · 3 comments
Closed

Unnecessary rerender/readding for existing messages #17

LKay opened this issue Nov 15, 2016 · 3 comments
Labels

Comments

@LKay
Copy link

LKay commented Nov 15, 2016

In this fragment you remove all notifications that are no longer in the state, which is totally fine.

    // Get all active notifications from react-notification-system
    /// and remove all where uid is not found in the reducer
    (this.system().state.notifications || []).forEach(notification => {
      if (notificationIds.indexOf(notification.uid) < 0) {
        this.system().removeNotification(notification.uid);
      }
    });

But here you are looping for each notifications in the redux state even if some are already displayed. You should filter the redux notifications against notification that are already displayed and add only new ones.

    notifications.forEach(notification => {
      this.system().addNotification({
        ...notification,
        onRemove: () => {
          this.context.store.dispatch(actions.hide(notification.uid));
          notification.onRemove && notification.onRemove();
        }
      });
    });
@gor181
Copy link
Owner

gor181 commented Nov 16, 2016

Hey @LKay ,

Good spot. Will check on how to solve this soon.

@gor181
Copy link
Owner

gor181 commented Dec 7, 2016

Hey @LKay ,

I think this was done on purpose considering react-notification-system is ignoring notifications with same id anyways so we do not need to filter here.

uid

Overrides the internal uid. Useful if you are managing your notifications id. Notifications with same uid won't be displayed.

Let me know if you are seeing duplicates being added which shouldn't be the case.

gor181 added a commit that referenced this issue Dec 7, 2016
Should not add notification if it already exists in the notification system based on the uid
see #17
@gor181 gor181 added the question label Dec 7, 2016
@gor181
Copy link
Owner

gor181 commented Dec 7, 2016

Hey @LKay

I'm closing this one as I have added tests which are confirming that no additional notifications are being added.

thanks!

@gor181 gor181 closed this as completed Dec 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants