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

fix: update naming of markAllNotifications methods in react NC #3521

Merged
merged 1 commit into from
May 29, 2023

Conversation

jainpawan21
Copy link
Member

What change does this PR introduce?

  1. Changed:-
    markAllNotificationsAsRead => markFetchedNotificationsAsRead
    markAllNotificationsAsSeen => markFetchedNotificationsAsSeen
    markAllNotificationsAsReadByFeed => markAllNotificationsAsRead

  2. Added:-
    markAllNotificationsAsSeen

  3. Update Docs

Why was this change needed?

Other information (Screenshots)

@linear
Copy link

linear bot commented May 28, 2023

NV-2390 correct naming of markAllNotifications methods in NC

What?

useNotifications hook in react NC provides these methods:-

  1. markAllNotificationsAsRead
  2. markAllNotificationsAsSeen
  3. markAllNotificationsAsReadByFeed

Why? (Context)

user may think that markAllNotificationsAsRead will mark all notifications as read but it does not, It only mark fetched notifications as read. Similarly markAllNotificationsAsSeen for seen.

Rather markAllNotificationsAsRead mark all notifications as read irrespective of it is fetched in view or not.

Definition of Done

Correct naming of these methods as:-

markAllNotificationsAsRead => markFetchedNotificationsAsRead

markAllNotificationsAsSeen => markFetchedNotificationsAsSeen

markAllNotificationsAsReadByFeed => markAllNotificationsAsRead

so that users don't get confused in first reading

@jainpawan21 jainpawan21 requested review from BiswaViraj and LetItRock and removed request for BiswaViraj May 28, 2023 17:35
@github-actions github-actions bot added @novu/api @novu/notification-center documentation Improvements or additions to documentation labels May 28, 2023
@jainpawan21 jainpawan21 requested review from davidsoderberg, BiswaViraj, scopsy and LetItRock and removed request for LetItRock and davidsoderberg May 28, 2023 17:35
@jainpawan21 jainpawan21 changed the title feat: add mark all notifications as seen fix: update naming of markAllNotifications methods in react NC May 28, 2023
Copy link
Contributor

@LetItRock LetItRock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome 🏆

@ainouzgali
Copy link
Contributor

Should we also add a change to headless ? To avoid confusion ? @jainpawan21

@jainpawan21
Copy link
Member Author

Should we also add a change to headless ? To avoid confusion ? @jainpawan21

Do you mean this method in headless?
https://docs.novu.co/notification-center/headless/api-reference#markallmessagesasread

@BiswaViraj wdyt, how this function is behaving? is it marking all or fetched?

@BiswaViraj
Copy link
Contributor

@jainpawan21, The methods in the headless package are correct, I changed them while I worked on it.
No changes needed in the headless package.

@jainpawan21
Copy link
Member Author

@ainouzgali @BiswaViraj shall I merge this PR?

@jainpawan21 jainpawan21 added this pull request to the merge queue May 29, 2023
Merged via the queue into next with commit 31b8f00 May 29, 2023
31 checks passed
@jainpawan21 jainpawan21 deleted the NV-2390 branch May 29, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation @novu/api @novu/notification-center
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants