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

Replace notifications toggle with a link to the settings app. #4575

Merged
merged 13 commits into from
Aug 3, 2021

Conversation

pixlwave
Copy link
Member

@pixlwave pixlwave commented Jul 19, 2021

Fixes #2368. Depends on matrix-org/matrix-ios-kit#865.

Outdated:

  • Removes the notifications toggle as it wasn't detecting some possible notification configurations, and proclaiming to the user that notifications were disabled when they weren't.
  • Ensures that notifications are enabled on launch. This will guarantee that everything is set up if the user enables them in the settings app at a later date.
  • Replaces the toggle cell with a link that opens up the Element section of the Settings app for easier configuration.

@pixlwave
Copy link
Member Author

Enabling notifications with this PR implemented:

New_480.mov

The same, but using the old toggle:

Old_480.mov

@niquewoodhouse
Copy link

After discussing, we agreed that whilst this fixes a lot of issues with notifications, it makes the primary flow a bit harder - as to turn off notifications now would take a bit longer and more interactions. When someone is trying to turn on/off notifications, it's normally a time-sensitive task, and taking longer than expected there would be a new papercut.

We agreed to keep the exisitng toggle for the quick turn on off and introduce a way to device notifications.

Reference (as it is now, just for reference)

Current@2x

Option 1

Option 1@2x

Option 2

Option 2@2x

Option 3

Option 3@2x

@pixlwave pixlwave force-pushed the doug/2368_allow_silent_notifications branch from 1f20611 to 88660d5 Compare July 28, 2021 09:10
@pixlwave
Copy link
Member Author

Updated to include the original toggle as well as the new link to the system settings app. The behaviour of the old toggle has been updated to include support for background notifications.

NotificationSettings2-480p.mov

@niquewoodhouse
Copy link

@pixlwave could we amend to alert to be more actionable, with a cancel and a settings action that goes to the relevant device notifications?

ios-notifications-enabled-01

@pixlwave
Copy link
Member Author

Yep, much better!

NotificationsAlert-480p.mov

@pixlwave pixlwave marked this pull request as ready for review July 28, 2021 11:49
Copy link

@niquewoodhouse niquewoodhouse left a comment

Choose a reason for hiding this comment

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

lgtm

@pixlwave
Copy link
Member Author

Summary of final changes:

  • Updates the notifications toggle to detect modern notification configurations.
  • Update alert when notifications have been disabled in the Settings app.
  • Add a link to open the Element section of the Settings app for easier notification configuration.

@pixlwave pixlwave merged commit cc50f52 into develop Aug 3, 2021
@pixlwave pixlwave deleted the doug/2368_allow_silent_notifications branch August 3, 2021 15:03
@manuroe manuroe mentioned this pull request Aug 9, 2021
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.

Notifications should be able to "Deliver Quietly"
3 participants