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

Sync system notification settings; filter out Abacus notifications #258

Merged
merged 16 commits into from
Sep 18, 2024

Conversation

ruixhuang
Copy link
Contributor

@ruixhuang ruixhuang commented Sep 18, 2024


Description / Intuition

Various updates for push notifications:

  • Sync system notification settings so that it's consistent with app setttings
  • Change the default notification color to grey as in Figma spec.
  • Filter out Abacus notifications for order status and position update
  • Add a dismiss button to primer

Simulator Screenshot - iPhone 15 Pro (17 5) - 2024-09-18 at 11 12 33

Simulator Screenshot - iPhone 15 Pro - 2024-09-17 at 13 08 35


Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring or Technical Debt
  • Documentation update
  • Other (please describe: )

@ruixhuang ruixhuang marked this pull request as ready for review September 18, 2024 18:13
if successful.boolValue {
if let self = self {
self.keyValueStore?.setValue(code, forKey: self._languageTag)
let localizer = UIImplementations.shared?.localizer as? DynamicLocalizer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add some logging; no functional change

import RoutingKit
import Utilities

public final class dydxPushNotifcationToggleWorker: BaseWorker {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Notification*

Comment on lines +49 to +51
viewModel?.cancelAction = {
Router.shared?.navigate(to: RoutingRequest(path: "/action/dismiss", params: nil), animated: true, completion: nil)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙏

@@ -57,7 +65,10 @@ private class dydxNotificationsSettingsViewPresenter: SettingsViewPresenter {
cancelTitle: DataLocalizer.shared?.localize(path: "APP.GENERAL.CANCEL", params: nil) ?? "Cancel"
)
}
// sync the settings with the system permission
reloadSettings()
Copy link
Collaborator

Choose a reason for hiding this comment

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

would this get called while user is being prompted to turn on notifications? Does this have an effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah.. it gets called, which sets the value back... But when the system notification value changes, it will be synced again.

@ruixhuang ruixhuang merged commit 56a8913 into develop Sep 18, 2024
5 checks passed
@ruixhuang ruixhuang deleted the features/push_2 branch September 18, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants