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

feat(plugin): ToastNotifications #1806

Draft
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

ImSkully
Copy link

@ImSkully ImSkully commented Oct 16, 2023

Adds configurable toast notifications that are sent whenever the user receives a message in direct messages, group chats, or gets pinged anywhere. The pop-up notification is an expansion of the existing NotificationComponent to add support for a variety of more features and making them configurable within the plugin settings.

Configurable Options

  • Notification position (any corner of the screen)
  • Visibility duration/timeout
  • Notification pane opacity
  • Maximum number of notifications to display at a time

👀Preview

ToastNotifications.mp4

Users can adjust the visual appearance of notifications & it's very easily themed by adjusting the normalized CSS classes which are used across notification elements.

Development

This is an early draft of the current release, I'd like to implement all of the features below & clean-up the code a bit more before this is ready for use.

🛠 Completion Requirements

  • Figure out plugin notification component vs. existing Vencord Notification API1
  • Fix height offset not using actual notification height (message content increases height which is not currently being accounted for)
  • Swap to enum/constant references where possible
  • Improve emoji rendering
  • Add a max string length and truncate when body is too big
  • Improve notification styling (can definitely be condensed more to not be as large)
  • Create README.md which also includes some help for theming via ToastNotification CSS classes

✨Additional Features

  • Add "Ignore DND Mode" setting: avoids sending notifications when in DND mode unless this is enabled
  • Add support for notifications appearing when pinged in any server or channel
  • Add option to right-click notifications to mark conversation as read
  • Format plugin settings pane with better descriptions
  • Use DOM message component from messages directly and re-render in notification2

Footnotes

  1. https://github.com/Vendicated/Vencord/pull/1806#issuecomment-1793610778

  2. https://github.com/Vendicated/Vencord/pull/1806#issuecomment-1763567597

@Vendicated
Copy link
Owner

Vendicated commented Oct 16, 2023

you should use Vencord's notification api instead of making your own

if you want additional features, edit it to add them; but the first two options you state are already native features and the opacity can easily be done with css

@ImSkully
Copy link
Author

ImSkully commented Oct 16, 2023

Thanks for your input, originally I did make this using Vencord's Notification API though realized that this would conflict with notifications from Vencord itself, and other plugins that may make use of it which would be conflicting.

The other primary conflict is the style of the notification: I plan to adjust the core style of the notification to be very different to how it appears now, along with making it easily adjustable for user's to theme themselves. If we share the same Notification API then it would also be adjusting Vencord's notifications which may not be ideal, especially for other plugin developers who want to use the built-in notifications the way they currently are.

@Vendicated
Copy link
Owner

UI should be consistent so making your own different notifications is meh. it is also probably the best idea to just render the normal message component inside the notification to save a lot of work, in which case it would hardly even matter

@ImSkully
Copy link
Author

ImSkully commented Nov 5, 2023

@Vendicated

you should use Vencord's notification api instead of making your own

if you want additional features, edit it to add them; but the first two options you state are already native features and the opacity can easily be done with css

Just adding on this after some further deliberation on the best way to handle this following your suggestion, I did some work locally on improving Vencord's notification API to support the features that I need for this plugin however there are too many conflicts in the functionality of notifications if we are to restrict plugins to all use the same API for the purpose of displaying client notifications.

My implementation adds a configurable notification 'stack' size which adjusts how many notifications are shown on screen at any given time - any newer notification beyond the max size will animate the oldest message out and remove it from the stack.

If every developer is to use the same notification API for all plugins, along with Vencord itself, then all notifications would be sharing the same stack and inevitably this will result in plugins pushing out notifications created by other plugins. Sure I could implement plugin-specific channels so they don't share the same message queue but then how do we handle rendering all of these notifications on the client?

For instance, in the intended functionality of my plugin, if the user gets 3 private messages then that's 3 notifications visible on screen; where do we now display notifications that a different plugin may want to potentially show at the same time? How do we handle the additional behaviour of permanent notifications that don't automatically timeout? (would require some form of notification prioritization within the restricted notification queue size)

Beyond that, this also restricts creating custom functionality to the notifications' behaviour on a per-plugin basis since the notifications' position, opacity, timeout, etc. will all be adjustable in the existing notification style section within Vencord settings.

Ideally, users should be free to customize the settings of notifications from each source so they can arrange them with whatever behaviour they desire: I would personally like to see my plugin's notifications at the bottom left with a timeout of 5 seconds and a max queue size of 3, and notifications from the existing API at the top left with no timeout and only ever see one at a time until dismissed.

UI should be consistent so making your own different notifications is meh.

I completely agree, the appearance of notifications should be pretty much standardized for consistency, however, we should still allow the user to customize them via CSS as I've done so by providing CSS classes for each element of the notification UI - this means users can freely change the appearance of notifications from this plugin without affecting those from the base notification API.

With that being said, let me know if you're happy for me to go ahead with using the notification component I made for this plugin since I've halted progressing this any further until this discussion is resolved.

it is also probably the best idea to just render the normal message component inside the notification to save a lot of work

This is something I originally tried to do but I'm not a frontend dev so I have no experience with JSX/React/DOM manipulation in order to get the original message component and just re-render that - if you can give me an example implementation on how this is done or point me in the right direction then I'll definitely give it a go.

@ImSkully
Copy link
Author

@Vendicated bump

@ImSkully
Copy link
Author

@Vendicated bump

3 similar comments
@ImSkully
Copy link
Author

ImSkully commented Dec 3, 2023

@Vendicated bump

@ImSkully
Copy link
Author

ImSkully commented Dec 9, 2023

@Vendicated bump

@Najmul190
Copy link

@Vendicated bump

@ItsZil
Copy link

ItsZil commented Dec 21, 2023

Bump, would love to have this.

@belg1o
Copy link

belg1o commented Dec 28, 2023

@Vendicated

you should use Vencord's notification api instead of making your own
if you want additional features, edit it to add them; but the first two options you state are already native features and the opacity can easily be done with css

Just adding on this after some further deliberation on the best way to handle this following your suggestion, I did some work locally on improving Vencord's notification API to support the features that I need for this plugin however there are too many conflicts in the functionality of notifications if we are to restrict plugins to all use the same API for the purpose of displaying client notifications.

My implementation adds a configurable notification 'stack' size which adjusts how many notifications are shown on screen at any given time - any newer notification beyond the max size will animate the oldest message out and remove it from the stack.

If every developer is to use the same notification API for all plugins, along with Vencord itself, then all notifications would be sharing the same stack and inevitably this will result in plugins pushing out notifications created by other plugins. Sure I could implement plugin-specific channels so they don't share the same message queue but then how do we handle rendering all of these notifications on the client?

For instance, in the intended functionality of my plugin, if the user gets 3 private messages then that's 3 notifications visible on screen; where do we now display notifications that a different plugin may want to potentially show at the same time? How do we handle the additional behaviour of permanent notifications that don't automatically timeout? (would require some form of notification prioritization within the restricted notification queue size)

Beyond that, this also restricts creating custom functionality to the notifications' behaviour on a per-plugin basis since the notifications' position, opacity, timeout, etc. will all be adjustable in the existing notification style section within Vencord settings.

Ideally, users should be free to customize the settings of notifications from each source so they can arrange them with whatever behaviour they desire: I would personally like to see my plugin's notifications at the bottom left with a timeout of 5 seconds and a max queue size of 3, and notifications from the existing API at the top left with no timeout and only ever see one at a time until dismissed.

UI should be consistent so making your own different notifications is meh.

I completely agree, the appearance of notifications should be pretty much standardized for consistency, however, we should still allow the user to customize them via CSS as I've done so by providing CSS classes for each element of the notification UI - this means users can freely change the appearance of notifications from this plugin without affecting those from the base notification API.

With that being said, let me know if you're happy for me to go ahead with using the notification component I made for this plugin since I've halted progressing this any further until this discussion is resolved.

it is also probably the best idea to just render the normal message component inside the notification to save a lot of work

This is something I originally tried to do but I'm not a frontend dev so I have no experience with JSX/React/DOM manipulation in order to get the original message component and just re-render that - if you can give me an example implementation on how this is done or point me in the right direction then I'll definitely give it a go.

@Vendicated could you please look into this and consider this. I am sure many people would love to use an InAppNotification system, it's one of BD's most downloaded plugins. Sitting at over 525K downloads.

@ethan-davies
Copy link

@Vendicated Bump

Repository owner locked and limited conversation to collaborators Dec 29, 2023
Repository owner unlocked this conversation May 12, 2024
@ImSkully
Copy link
Author

@Vendicated Appreciate you unlocking this PR, however I still need a response to my previous message regarding the approach you want me to take to implement notifications. I have attempted to get your response for this PR on numerous occasions in the last 7 months, all of which were ignored.

The outcome of whether or not you allow this plugin to use it's own notifications instead of Vencord's notification API heavily determines how much of a refactor is required and prevents further development given how much of a functional reliance there is based on what is decided.

@AMufInABox
Copy link

Is this still being worked on?

@ImSkully
Copy link
Author

Still waiting on a response to my previous comment - #1806 (comment)

@AMufInABox
Copy link

@Vendicated bump ^

@Feathers8
Copy link

Nah, don't do this, or it'll get locked again. 😅

@AMufInABox
Copy link

AMufInABox commented Aug 10, 2024 via email

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.

9 participants