-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Sidebar: Minor Improvements #23611
Sidebar: Minor Improvements #23611
Changes from 10 commits
7cbfcf3
3c806e7
7bd49cd
78a2193
6fbc07f
28610dc
8fed06c
be365ad
49cff0b
94683f6
58407bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,8 +57,8 @@ final class SplitViewRootPresenter: RootViewPresenter { | |
self?.sidebarViewModel.selection = .blog(TaggedManagedObjectID(site)) | ||
}.store(in: &cancellables) | ||
|
||
// -warning: List occasionally sets the selection to `nil` when switching items. | ||
sidebarViewModel.$selection.compactMap { $0 } | ||
.removeDuplicates() | ||
.sink { [weak self] in self?.configure(for: $0) } | ||
.store(in: &cancellables) | ||
|
||
|
@@ -100,12 +100,10 @@ final class SplitViewRootPresenter: RootViewPresenter { | |
siteContent = SiteSplitViewContent(blog: site) | ||
content = siteContent! | ||
} catch { | ||
// TODO: (wpsidebar) switch to a different blog? | ||
return | ||
return wpAssertionFailure("selected blog not found") | ||
} | ||
} | ||
case .notifications: | ||
// TODO: (wpsidebar) update tab bar item when new notifications arrive | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already do it in |
||
if let notificationsContent { | ||
content = notificationsContent | ||
} else { | ||
|
@@ -123,6 +121,8 @@ final class SplitViewRootPresenter: RootViewPresenter { | |
|
||
display(content: content) | ||
|
||
// The `main.async` call fixed an issue where sometimes the sidebar doesn't | ||
// update the displayed selection in the list after switching to a new item | ||
DispatchQueue.main.async { | ||
self.splitVC.hide(.primary) | ||
} | ||
|
@@ -135,6 +135,10 @@ final class SplitViewRootPresenter: RootViewPresenter { | |
return navigationVC | ||
} | ||
|
||
private func makeErrorViewController() -> UIViewController { | ||
UIHostingController(rootView: EmptyStateView(SharedStrings.Error.generic, systemImage: "exclamationmark.circle")) | ||
} | ||
|
||
private func navigate(to step: SidebarNavigationStep) { | ||
switch step { | ||
case .allSites(let sourceRect): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,15 +33,18 @@ final class ReaderSidebarViewController: UIHostingController<AnyView> { | |
func showInitialSelection() { | ||
cancellables = [] | ||
|
||
viewModel.$selection.sink { [weak self] in | ||
self?.configure(for: $0) | ||
}.store(in: &cancellables) | ||
// -warning: List occasionally sets the selection to `nil` when switching items. | ||
viewModel.$selection.compactMap { $0 } | ||
.removeDuplicates { [weak self] in | ||
guard $0 == $1 else { return false } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is the easiest way to implement it without introducing any additional state. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think implementing it by handing double tap gesture? At the moment, double tap would lead to It's very possible that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It has to be single tap because that's what Apple apps do.
That's a good point. I'm not sure how to address it because the way There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's OK to ship it this way because if it's a deep link for let's say "Discover", it should be OK to pop to root. Both the deep links and Split View needs to be revised in the future versions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only checked out Apple Podcasts app. And it has inconsistent UX. If you tap one of the tabs (i.e. Browse), the app takes you to the root. But if you tap other items (like in the Library section), it doesn't. I feel like Apple apps may piggy pack on UITabBarController's implementation.
Deep link is just one example on top of my head. From the code level, I guess it's okay to ship it as the potential issue is limited on the Reader tab only. |
||
self?.popSecondaryViewControllerToRoot() | ||
return true | ||
} | ||
.sink { [weak self] in self?.configure(for: $0) } | ||
.store(in: &cancellables) | ||
} | ||
|
||
private func configure(for selection: ReaderSidebarItem?) { | ||
guard let selection else { | ||
return | ||
} | ||
private func configure(for selection: ReaderSidebarItem) { | ||
switch selection { | ||
case .main(let screen): | ||
showSecondary(makeViewController(for: screen)) | ||
|
@@ -58,6 +61,11 @@ final class ReaderSidebarViewController: UIHostingController<AnyView> { | |
} | ||
} | ||
|
||
private func popSecondaryViewControllerToRoot() { | ||
let secondaryVC = splitViewController?.viewController(for: .secondary) | ||
(secondaryVC as? UINavigationController)?.popToRootViewController(animated: true) | ||
} | ||
|
||
private func makeViewController<T: ReaderAbstractTopic>(withTopicID objectID: TaggedManagedObjectID<T>) -> UIViewController { | ||
do { | ||
let topic = try viewContext.existingObject(with: objectID) | ||
|
@@ -199,6 +207,19 @@ private struct ReaderSidebarView: View { | |
.toolbar { | ||
EditButton() | ||
} | ||
.tint(preferredTintColor) | ||
} | ||
|
||
private var preferredTintColor: Color { | ||
if #available(iOS 18, *) { | ||
return AppColor.tint | ||
} else { | ||
// This is a workaround for an iOS issue where it will not apply the | ||
// correrect colors in dark mode when the sidebar is displayed in a | ||
// supplementary column. If use use black as a tint color, it | ||
// displays white text on white background | ||
return Color(UIColor(light: UIAppColor.tint, dark: .secondaryLabel)) | ||
} | ||
} | ||
|
||
@ViewBuilder | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to a tech debt list.