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-52219 Add announcement bar to prompt users for notification permission #26849

Merged
merged 11 commits into from
May 21, 2024

Conversation

hmhealey
Copy link
Member

@hmhealey hmhealey commented Apr 23, 2024

Summary

This is a fix for the issue where some browsers prevent us from prompting for notification permission because the app tries to notify the user when the browser isn't focused.

My original plan was for something fancier where we'd wait until the user could've received a notification and then we prompt them for permission when the app next gets focus, but that ended up getting overly complicated, and it changed the existing code which had me worried that something might break.

Instead, I went for the super simple solution of just showing a "give us permission" banner as soon as the app opens like other apps do. This might be mildly annoying to people, but it's simple and hopefully effective enough to do the trick. It also leaves the existing code path in place so, if someone ignores the banner, they'll still get the same permission prompt that we previously trigger.

Ticket Link

MM-52219

Release Note

Added banner to prompt users to give desktop notification permission when opening the app

@hmhealey hmhealey added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review 1. UX Review Requires review by a UX Designer labels Apr 23, 2024
@mattermost-build
Copy link
Contributor

E2E tests not automatically triggered, because the PR is not in a mergeable state. Please update the branch with the base branch and resolve outstanding conflicts.

@mm-cloud-bot
Copy link

@hmhealey: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

3 similar comments
@mm-cloud-bot
Copy link

@hmhealey: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

@mm-cloud-bot
Copy link

@hmhealey: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

@mm-cloud-bot
Copy link

@hmhealey: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

@mm-cloud-bot mm-cloud-bot added do-not-merge/release-note-label-needed release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed labels Apr 23, 2024
@hmhealey
Copy link
Member Author

@abhijit-singh Let me know if there's any changes you'd want to see from a user perspective, particularly around the timing the bar appears, the text on the bar, and whether or not we should have a "don't ask me again" button. I didn't add that last one because it seemed redundant with just denying permission, but I'm 0/5 on that.

@abhijit-singh abhijit-singh added the Setup Cloud Test Server Setup an on-prem test server label Apr 26, 2024
@mm-cloud-bot
Copy link

Creating a new SpinWick test server using Mattermost Cloud.

@mm-cloud-bot
Copy link

Mattermost test server created! 🎉

Access here: https://mattermost-pr-26849.test.mattermost.cloud

Account Type Username Password
Admin sysadmin Sys@dmin123
User user-1 User-1@123

Your Spinwick's installation ID is: q93h84dg5384ucoiw18fi3chgo
To access the logs, please click here

Copy link

@abhijit-singh abhijit-singh left a comment

Choose a reason for hiding this comment

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

Thanks @hmhealey! I have a few thoughts:

  1. I don't think the permissions banner should show even before the user has logged in. I'd recommend we should ideally tie it to a basic user action like switching a channel (open to other ideas). As soon as the user logs in, they already see the onboarding flow right now which we shouldn't distract them from. Switching a channel shows intent of using the app.
Screenshot 2024-04-29 at 6 03 31 PM Screenshot 2024-04-29 at 6 06 33 PM
  1. I think we can update the language a bit here and not mention Mattermost since there's some work being done around comprehensive white labeling. I'd recommend something like: "We need your permission to show desktop notifications and keep you updated on new messages" with a CTA of Enable notifications

cc @cwarnermm

@hmhealey hmhealey added the Awaiting Submitter Action Blocked on the author label Apr 29, 2024
@harshilsharma63
Copy link
Member

/update-branch

@hmhealey hmhealey added Awaiting Submitter Action Blocked on the author and removed Awaiting Submitter Action Blocked on the author labels May 13, 2024
@hmhealey
Copy link
Member Author

@abhijit-singh I ended up just removing the z-index from that component entirely since, as far as I can tell, that hasn't been needed for a long time. I think that's a remnant of when the announcement bar used to sometimes overlap the app, and it's no longer needed ever since Caleb made the whole app use CSS grid a couple years ago

Copy link

@abhijit-singh abhijit-singh left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @hmhealey!

@hmhealey hmhealey removed 2: Dev Review Requires review by a developer 1. UX Review Requires review by a UX Designer labels May 15, 2024
@yasserfaraazkhan
Copy link
Contributor

@hmhealey If user blocks this and reloads browser. The banner won't show again. Is this expected?

Screenshot 2024-05-16 at 4 35 42 PM
  • If we allow it from here. The notifications are on, but the banner persists. Unless we reload.
image

@hmhealey
Copy link
Member Author

Yeah, those are both expected.

For the first one, we can't prompt the user to change their permission if they've previously blocked it, so I don't bother showing the bar.

For the second one, we can't detect that the user changed their permissions from outside the web app, so it's normal that the bar doesn't disappear unless the user specifically clicks on "Enable notifications" on it. From my testing, that also matches how Slack behaves

@mm-cloud-bot
Copy link

New commit detected. SpinWick will upgrade if the updated docker image is available.

@mm-cloud-bot
Copy link

Mattermost test server updated with git commit 19744de862eb68bbf2d0fe53b92e4a11bb8cd642.

Access here: https://mattermost-pr-26849.test.mattermost.cloud

@hmhealey hmhealey added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels May 21, 2024
@hmhealey hmhealey merged commit 3792f1d into master May 21, 2024
25 checks passed
@hmhealey hmhealey deleted the MM-52219_fix-notification-prompt-1 branch May 21, 2024 17:54
@mattermost-build mattermost-build removed the Setup Cloud Test Server Setup an on-prem test server label May 21, 2024
@amyblais amyblais added Docs/Needed Requires documentation Changelog/Done Required changelog entry has been written labels May 21, 2024
@amyblais amyblais added this to the v9.10.0 milestone May 21, 2024
@amyblais amyblais added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written Docs/Done Required documentation has been written release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants