Skip to content

Conversation

anthonyss09
Copy link

@anthonyss09 anthonyss09 commented Sep 13, 2022

line 480 the original filter function returned an array notifications where !notification.read was the condition. Due to the second render of the NotificationList component allNotificationsRead is being called in useLayoutEffect and notifications.filter(notification=>!notification.read).length becomes 0. So we should return a notifications array from the filter function with the condition notifications.isNew.


name: 🐛 Bug fix or new feature
about: Fixing a problem with Redux

PR Type

Does this PR add a new feature, or fix a bug?

Why should this PR be included?

Checklist

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Is there an existing issue for this PR?
    • link issue here
  • Have the files been linted and formatted?
  • Have the docs been updated to match the changes in the PR?
  • Have the tests been updated to match the changes in the PR?
  • Have you run the tests locally to confirm they pass?

New Features

What new capabilities does this PR add?

What docs changes are needed to explain this?

Bug Fixes

What is the current behavior, and the steps to reproduce the issue?

What is the expected behavior?

How does this PR fix the problem?

line 480 when the original filter returned notifications where !notification.read was the condition, due to the second render of <NotificationList/> allNotificationsRead is being called in useLayoutEffect and notifications.filter(n=>!n.read) is 0.
So we should filter based upon isNew.
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c796225:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration

@netlify
Copy link

netlify bot commented Sep 13, 2022

Deploy Preview for redux-docs ready!

Name Link
🔨 Latest commit c796225
🔍 Latest deploy log https://app.netlify.com/sites/redux-docs/deploys/631ff0c5da64f70008665eac
😎 Deploy Preview https://deploy-preview-4418--redux-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@timdorr
Copy link
Member

timdorr commented Sep 13, 2022

This is correct as-is. The intended UX is the number of "unread" notifications are in the NavBar, not the number of "new" notifications. This should go to 0 when you click on the tab, displaying NotificationList.

@timdorr timdorr closed this Sep 13, 2022
@anthonyss09
Copy link
Author

anthonyss09 commented Sep 14, 2022

This is correct as-is. The intended UX is the number of "unread" notifications are in the NavBar, not the number of "new" notifications. This should go to 0 when you click on the tab, displaying NotificationList.

I see... I didn't realize the notifications badge was not intended to be present whilst viewing the notifications, but that makes sense. Thanks for your time!
Also I enjoyed this tutorial very much!

@markerikson
Copy link
Contributor

@anthonyss09 thank you, glad to hear it was helpful!

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 this pull request may close these issues.

3 participants