chore: Notification preference settings and relevant code #104
Conversation
|
|
||
| var errorMessage: String? { | ||
| didSet { | ||
| withAnimation { |
There was a problem hiding this comment.
bad practice: openedx#277
needs to move animations inside view
| @Published var showError: Bool = false | ||
| public var hasPermission: Bool = false | ||
| private var interactor: NotificationsInteractorProtocol | ||
| private var router: NotificationsRouter |
There was a problem hiding this comment.
Do we plan to perform routing from this view?
|
|
||
| public class NotificationsSettingsViewModel: ObservableObject { | ||
| @Published var showError: Bool = false | ||
| public var hasPermission: Bool = false |
There was a problem hiding this comment.
Since we will have a list of push settings, perhaps we need to develop a way to interact with the list of ‘settings’.
Same in View - we need to have a ForEach that will show a list of all settings (I realise we don't have an API now, but we need to interact with many settings, and for the API we could just have an empty function at the moment). Just add one/two more settings to the existing ‘Discussion Activities'.
There was a problem hiding this comment.
Yes, in the future, we will be getting more push notification settings, but when we are not sure, that's why I keep it simple as per the current requirements. When they come, we will plan it accordingly.
| .highPriorityGesture( | ||
| DragGesture(minimumDistance: 20, coordinateSpace: .local).onEnded { _ in | ||
| viewModel.toggleNotificationsPermissionAction() | ||
| } | ||
| ) | ||
| .highPriorityGesture( | ||
| TapGesture().onEnded { | ||
| viewModel.toggleNotificationsPermissionAction() | ||
| } | ||
| ) |
There was a problem hiding this comment.
We added PriorityGesture here to fix iOS18 bug when parental View intercepts gestures and when we use Toggle not just like 'toggle'. Do you see the same here? Otherwise we don't need . highPriorityGesture here. Please look. Thank you
|
@saeedbashir Please update the navigation bar design to match the Figma designs. |
There was a problem hiding this comment.
Please remove this file as it's duplicate here.
|
|
||
| var errorMessage: String? { | ||
| didSet { | ||
| showError = errorMessage != nil |
saeedbashir
force-pushed
the
saeed/LEARNER-10312
branch
from
93128d2 to
5e4a65c
Compare
Description
Add push notifications settings in the profile settings to change the permission of discussion activities to send the notifications messages.
This PR only adds the UI as the preference API isn't ready yet.
Config PR: https://github.com/edx/edx-mobile-config/pull/188
Screenshots: