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

[NV-2150] 🐛 Bug Report: NotificationBell ignores unseenCount #3246

Open
2 tasks done
david-morris opened this issue Apr 19, 2023 · 2 comments
Open
2 tasks done

[NV-2150] 🐛 Bug Report: NotificationBell ignores unseenCount #3246

david-morris opened this issue Apr 19, 2023 · 2 comments

Comments

@david-morris
Copy link
Contributor

david-morris commented Apr 19, 2023

📜 Description

NotificationBell ignores unseenCount when the context is available.

This is bad because it makes it impossible to use the built in notification bell when you want to change the effective value of unseenCount, e.g. when you want to use different feeds and only "count" one of them.

👟 Reproduction steps

  1. Connect to novu with a NovuProvider
  2. Inside the NovuProvider, use a NotificationBell inside PopoverNotificationCenter
  3. hardcode a value for unseenCount of NotificationBell.

👍 Expected behavior

NotificationBell should honor the count passed to it before looking at context.

👎 Actual Behavior with Screenshots

image
The total number of items is the value in context, not the value given.

📃 Provide any additional context for the Bug.

In general, the components you have are very useful for getting started, but it would be much nicer if they were designed to allow the user to wrap and change behavior, view, or both.

i.e. instead of using a context for the header, listItem, etc. props, and having one component calculate all the data and then just render the component in the context if that's provided, it would be nice to expose props and allow higher-order components there.

There's loads of work in the header, for example, and I don't want to have to throw out the whole settings widget and panel just because I want to change the displayed count of items. There's loads of work in the listItem renderer, and I don't want to have to rewrite it all just to conditionally add some react components into the message body.

I'm guessing that the unseenCount was either obsoleted, or designed for unit tests but someone writing the documentation didn't notice.

👀 Have you spent some time to check if this bug has been raised before?

  • I checked and didn't find similar issue

🏢 Have you read the Contributing Guidelines?

Are you willing to submit PR?

None

NV-2150

@davidsoderberg davidsoderberg changed the title 🐛 Bug Report: NotificationBell ignores unseenCount [NV-2150] 🐛 Bug Report: NotificationBell ignores unseenCount Apr 19, 2023
@gitstart
Copy link
Contributor

@david-morris I would like to pick this up cc @scopsy

@ainouzgali
Copy link
Contributor

@gitstart I feel this is something that requires more planning and discussion inside the team before it can be worked on.

@oba2311 - @david-morris made very good points, I think the heart of this issue is in the 📃 Provide any additional context for the Bug.

Thank you @david-morris @gitstart !

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