Skip to content

Commit

Permalink
Sidebar: Minor Improvements (#23611)
Browse files Browse the repository at this point in the history
* Update sidebar to use black

* Fix login buttons disabled state background

* ReaderSidebarViewController popToRootViewController when slecting the same item

* Remember Reader sidebar selection persistently

* Simplify how Reader implements popToRootViewController (remove currentlySelectedItem

* Remove focus effect

* Add assertion when blog cant be found

* Remove note about notifications (already doing it)

* Remove tech debt not about Site Menu

* Remove Me tab in wpios on iPad

* Remove unused makeErrorViewController
  • Loading branch information
kean committed Sep 20, 2024
1 parent 9ade235 commit 54da4a2
Show file tree
Hide file tree
Showing 10 changed files with 59 additions and 24 deletions.
10 changes: 10 additions & 0 deletions WordPress/Classes/Stores/UserPersistentRepositoryUtility.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ private enum UPRUConstants {
static let jetpackContentMigrationStateKey = "jetpackContentMigrationState"
static let mediaAspectRatioModeEnabledKey = "mediaAspectRatioModeEnabled"
static let readerSidebarSelectionKey = "readerSidebarSelectionKey"
static let isReaderSelectedKey = "isReaderSelectedKey"
}

protocol UserPersistentRepositoryUtility: AnyObject {
Expand Down Expand Up @@ -157,6 +158,15 @@ extension UserPersistentRepositoryUtility {
}
}

var isReaderSelected: Bool {
get {
UserPersistentStoreFactory.instance().bool(forKey: UPRUConstants.isReaderSelectedKey)
}
set {
UserPersistentStoreFactory.instance().set(newValue, forKey: UPRUConstants.isReaderSelectedKey)
}
}

var readerSidebarSelection: ReaderStaticScreen? {
get {
let repository = UserPersistentStoreFactory.instance()
Expand Down
1 change: 0 additions & 1 deletion WordPress/Classes/System/SplitViewRootPresenter+Site.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ class SiteSplitViewContent: SiteMenuViewControllerDelegate, SplitViewDisplayable
func displayed(in splitVC: UISplitViewController) {
RecentSitesService().touch(blog: blog)

// TODO: (wpsidebar) Refactor this (initial .secondary vc managed based on the VC presentation)
_ = siteMenuVC.view
}

Expand Down
8 changes: 4 additions & 4 deletions WordPress/Classes/System/SplitViewRootPresenter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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
if let notificationsContent {
content = notificationsContent
} else {
Expand All @@ -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)
}
Expand Down
11 changes: 4 additions & 7 deletions WordPress/Classes/Utility/App Configuration/AppColor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,12 @@ struct UIAppColor {
static let accent = CSColor.Pink.base

#if IS_JETPACK
static let tint = UIColor.label
static let brand = CSColor.JetpackGreen.base
#endif

#if IS_WORDPRESS
static let tint = brand
static let brand = CSColor.WordPressBlue.base
#endif

Expand Down Expand Up @@ -128,11 +130,6 @@ struct UIAppColor {
}

struct AppColor {
#if IS_JETPACK
static let brand = Color(CSColor.JetpackGreen.base)
#endif

#if IS_WORDPRESS
static let brand = Color(CSColor.WordPressBlue.base)
#endif
static let tint = Color(UIAppColor.tint)
static let brand = Color(UIAppColor.brand)
}
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ extension BlogDetailsViewController {
}

@objc func shouldAddMeRow() -> Bool {
return JetpackFeaturesRemovalCoordinator.currentAppUIType == .simplified
JetpackFeaturesRemovalCoordinator.currentAppUIType == .simplified && !isSidebarModeEnabled
}

@objc func shouldAddSharingRow() -> Bool {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,8 @@ extension WordPressAuthenticationManager {
secondaryNormalBorderColor: .systemGray3,
secondaryHighlightBackgroundColor: .systemGray3,
secondaryHighlightBorderColor: .systemGray3,
disabledBackgroundColor: UIAppColor.gray,
disabledBorderColor: UIAppColor.neutral(.shade10),
disabledBackgroundColor: .secondarySystemFill,
disabledBorderColor: .secondarySystemFill,
primaryTitleColor: .white,
secondaryTitleColor: .label,
disabledTitleColor: UIAppColor.neutral(.shade20),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
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))
Expand All @@ -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)
Expand Down Expand Up @@ -201,6 +209,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ private struct SidebarView: View {
}
}
.listStyle(.sidebar)
.tint(AppColor.tint)
.overlay(alignment: .bottom) {
SidebarProfileContainerView(viewModel: viewModel)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,16 @@ final class SidebarViewModel: ObservableObject {
self.account = try? WPAccount.lookupDefaultWordPressComAccount(in: self.contextManager.mainContext)
}
.store(in: &cancellables)

$selection.sink {
UserDefaults.standard.isReaderSelected = $0 == .reader
}.store(in: &cancellables)
}

private func resetSelection() {
let blog = Blog.lastUsedOrFirst(in: contextManager.mainContext)
if let blog {
if UserDefaults.standard.isReaderSelected {
selection = .reader
} else if let blog = Blog.lastUsedOrFirst(in: contextManager.mainContext) {
selection = .blog(TaggedManagedObjectID(blog))
} else {
selection = .welcome
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ private final class SiteMenuListViewController: BlogDetailsViewController {
container.pinSubviewToAllEdges(backgroundView, insets: UIEdgeInsets(top: 0, left: 16, bottom: 0, right: 16))
return container
}()
cell.focusStyle = .custom
cell.focusEffect = nil

return cell
}
Expand Down

0 comments on commit 54da4a2

Please sign in to comment.