Feature/navigation update #47
Conversation
| #if os(macOS) | ||
| ToolTableItem.navigation( | ||
| NavigationToolItem( | ||
| title: key, | ||
| didSelect: { | ||
| UserDefaultsToolDetailView( | ||
| viewModel: UserDefaultsToolDetailViewModel( | ||
| value: String(describing: value), | ||
| title: key, | ||
| userDefaults: userDefaults, | ||
| didDeleteProperty: { [unowned self] in sections = createSectionItems(with: userDefaults) } | ||
| ), | ||
| selection: $0 | ||
| ) | ||
| } | ||
| ) | ||
| ) | ||
| #else | ||
| ToolTableItem.navigation( | ||
| NavigationToolItem( | ||
| title: key, | ||
| didSelect: { | ||
| UserDefaultsToolDetailView( | ||
| viewModel: UserDefaultsToolDetailViewModel( | ||
| value: String(describing: value), | ||
| title: key, | ||
| userDefaults: userDefaults, | ||
| didDeleteProperty: nil // onAppear will update the screen on iOS | ||
| ) | ||
| ) | ||
| } | ||
| ) | ||
| ) | ||
| #endif |
There was a problem hiding this comment.
I would prefer moving didSelect into separate method, or just defining didDeleteProperty as a standalone variable and init the variable based on if-else.
There was a problem hiding this comment.
The difference between didSelect closures is messing up this one. The macOS one has a value provided in it which is then sent as the selection parameter, and the iOS one doesn't. Nothing comes to mind on how to handle this elegantly. Maybe you might think of something
| #if os(macOS) | ||
| selection = nil | ||
| #else | ||
| presentationMode.wrappedValue.dismiss() // on macOS dismisses the whole window which isn't desired |
There was a problem hiding this comment.
I've seen this used multiple times; can we instead make an extension that will handle this case by calling it as a one liner on a calling side and define inner logic in the extension?
There was a problem hiding this comment.
To extract this into a protocol or something we would still have to have if/else since the selection is hidden behind an if macos, and we would have to provide it somehow. It's called twice in the codebase, so for now, maybe we can leave it as is since macOS might be upped to version 13 so we can use NavigationStack
| @Environment(\.presentationMode) var presentationMode: Binding<PresentationMode> | ||
| @ObservedObject var viewModel: UserDefaultsToolDetailViewModel | ||
| #if os(macOS) | ||
| @Binding var selection: String? |
There was a problem hiding this comment.
Can we move this to the viewModel?
There was a problem hiding this comment.
Since it's a view specific binding, I feel like we should keep it here
| let title: String | ||
| let onValueChanged: (Bool) -> Void | ||
| let getter: () -> Bool |
There was a problem hiding this comment.
Which issue is fixed with adding a getter which is called on onAppear? Does item.loadStoredValue() not provide the correct value, and if not, in which cases?
There was a problem hiding this comment.
The issue was if we remove a value from the UserDefaultsTool which was in preferences, when we go back to the preferences it would keep the previous value since it was only instantiated once. This way we would check the value every time we enter the screen since it might have changed
Summary
I have update the current state of the UserDefaults tool to match the changes done in the preferences tab, and vice versa. Also, I have updated the macOS navigation so that the users can now pop the view in the NavigationSplitView that's used on macOS instead of being open for no reason.
Changes
The navigation changes are mostly done only for macOS since we needed to provide the selection binding from the SentinelListView.
The preference tab changes only added an onAppear to update the value if needed.
The UserDefaults changes are updating the view onAppear as well.
Type
Additional information
Description
Everything was mentioned in the changes section
Sentinel.updates.mov
Checklist
Additional notes