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

[MM-47256] Notifications native module #2290

Closed
wants to merge 26 commits into from

Conversation

tboulis
Copy link
Contributor

@tboulis tboulis commented Oct 18, 2022

Summary

Use node-notifier for displaying native notifications.
Add support for sending test notification under Desktop Settings page

Ticket Link

Screenshots

Release Note

Use native module for notifications

@tboulis tboulis added the Work In Progress Not yet ready for review label Oct 18, 2022
@tboulis tboulis force-pushed the MM-47256-native-notifications branch 2 times, most recently from 5504c33 to c10e4af Compare October 26, 2022 09:23
@tboulis tboulis marked this pull request as ready for review November 1, 2022 13:44
Copy link
Member

@devinbinnie devinbinnie left a comment

Choose a reason for hiding this comment

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

Overall looks good! Added a couple non-blocking comments.

src/main/preload/mattermost.js Show resolved Hide resolved
src/main/notifications/index.test.js Show resolved Hide resolved
@devinbinnie devinbinnie added the 2: Dev Review Requires review by a core committer label Nov 1, 2022
src/types/notification.ts Outdated Show resolved Hide resolved
@tboulis tboulis added the Build Apps for PR Builds signed builds for testing label Nov 3, 2022
@mattermod
Copy link
Contributor

Building app in separate branch.

@mattermod mattermod removed the Build Apps for PR Builds signed builds for testing label Nov 3, 2022
@tboulis tboulis added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer Work In Progress Not yet ready for review labels Nov 3, 2022
@jgilliam17
Copy link

Thanks @tboulis
Seeing one issue on Windows 10, I can hear the notification sound, but no notification popup for messages or when I test "send notification" in settings.
I did get "session expired" notifications, both sound and popup.
On macOS and Ubuntu this works as expected (sound and popup) for both new messages and testing the setting.

@tboulis
Copy link
Contributor Author

tboulis commented Nov 10, 2022

Thanks @tboulis Seeing one issue on Windows 10, I can hear the notification sound, but no notification popup for messages or when I test "send notification" in settings. I did get "session expired" notifications, both sound and popup. On macOS and Ubuntu this works as expected (sound and popup) for both new messages and testing the setting.

Thank you Jelena, I'll take a look!

@tboulis
Copy link
Contributor Author

tboulis commented Nov 14, 2022

The notifications on Windows dont seem to work after building the app. If I run the application locally they are displayed. It could be related to this issue: mikaelbr/node-notifier#182

@tboulis tboulis added the Blocked Blocked by external dependencies/teams label Nov 14, 2022
@tboulis tboulis force-pushed the MM-47256-native-notifications branch from ee081ba to 140c536 Compare December 21, 2022 13:06
@tboulis
Copy link
Contributor Author

tboulis commented Jan 5, 2023

node-notifier has too many issues. Closing..

Native module for Notifications feedback (node-notifier)

a. The notifications on Windows don't seem to work (not displayed) after building the app. If I run the application locally they are displayed. It could be related to this issue: mikaelbr/node-notifier#182. I tried the suggested solutions but it wasn't fixed
b. Dynamic notification images (user avatars) don't work correctly: mikaelbr/node-notifier#71
c. Cannot play custom sounds on all OSs, so we still need to play the sound manually, mikaelbr/node-notifier#166 (comment)
d.Notifications on macOS disappear mikaelbr/node-notifier#184

The above are major issues which lead me to move away from this package. Leaving this here for future reference.

Alternatives:

@tboulis tboulis closed this Jan 5, 2023
@tboulis tboulis deleted the MM-47256-native-notifications branch January 5, 2023 10:28
@tboulis tboulis removed the 3: QA Review Requires review by a QA tester label Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Blocked by external dependencies/teams release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants