Skip to content

Commit d8a560c

Browse files
keankean
authored
Reader: Menu improvements (#23668)
2 parents 39d05b9 + 732228d commit d8a560c

12 files changed

+161
-97
lines changed

‎WordPress/Classes/System/ReaderPresenter.swift‎

Lines changed: 33 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,9 @@ final class ReaderPresenter: NSObject, SplitViewDisplayable {
3737

3838
// TODO: (reader) update to allow seamless transitions between split view and tabs
3939
@objc func prepareForTabBarPresentation() -> UINavigationController {
40-
sidebarViewModel.isCompactStyleEnabled = true
40+
sidebarViewModel.isCompact = true
4141
mainNavigationController.navigationBar.prefersLargeTitles = true
4242
mainNavigationController.viewControllers = [sidebar]
43-
sidebar.navigationItem.backBarButtonItem = UIBarButtonItem(image: UIImage(systemName: "sidebar.left"), primaryAction: UIAction { [weak self] _ in
44-
self?.mainNavigationController.popViewController(animated: true)
45-
})
4643
sidebar.navigationItem.backButtonDisplayMode = .minimal
4744
showInitialSelection()
4845
return mainNavigationController
@@ -67,17 +64,17 @@ final class ReaderPresenter: NSObject, SplitViewDisplayable {
6764
private func configure(for selection: ReaderSidebarItem) {
6865
switch selection {
6966
case .main(let screen):
70-
showSecondary(makeViewController(for: screen))
67+
show(makeViewController(for: screen))
7168
case .allSubscriptions:
72-
showSecondary(makeAllSubscriptionsViewController(), isLargeTitle: true)
69+
show(makeAllSubscriptionsViewController(), isLargeTitle: true)
7370
case .subscription(let objectID):
74-
showSecondary(makeViewController(withTopicID: objectID))
71+
show(makeViewController(withTopicID: objectID))
7572
case .list(let objectID):
76-
showSecondary(makeViewController(withTopicID: objectID))
73+
show(makeViewController(withTopicID: objectID))
7774
case .tag(let objectID):
78-
showSecondary(makeViewController(withTopicID: objectID))
75+
show(makeViewController(withTopicID: objectID))
7976
case .organization(let objectID):
80-
showSecondary(makeViewController(withTopicID: objectID))
77+
show(makeViewController(withTopicID: objectID))
8178
}
8279
}
8380

@@ -133,13 +130,15 @@ final class ReaderPresenter: NSObject, SplitViewDisplayable {
133130

134131
private func makeAllSubscriptionsViewController() -> UIViewController {
135132
let view = ReaderSubscriptionsView() { [weak self] selection in
136-
guard let self else { return }
137-
let navigationVC = self.splitViewController?.viewController(for: .secondary) as? UINavigationController
138-
wpAssert(navigationVC != nil)
139133
let streamVC = ReaderStreamViewController.controllerWithTopic(selection)
140-
navigationVC?.pushViewController(streamVC, animated: true)
134+
self?.push(streamVC)
141135
}.environment(\.managedObjectContext, viewContext)
142-
return UIHostingController(rootView: view)
136+
let hostVC = UIHostingController(rootView: view)
137+
hostVC.title = ReaderSubscriptionsView.navigationTitle
138+
if sidebarViewModel.isCompact {
139+
hostVC.navigationItem.largeTitleDisplayMode = .never
140+
}
141+
return hostVC
143142
}
144143

145144
private func navigate(to item: ReaderSidebarNavigation) {
@@ -169,7 +168,9 @@ final class ReaderPresenter: NSObject, SplitViewDisplayable {
169168
UIHostingController(rootView: EmptyStateView(SharedStrings.Error.generic, systemImage: "exclamationmark.circle"))
170169
}
171170

172-
private func showSecondary(_ viewController: UIViewController, isLargeTitle: Bool = false) {
171+
/// Shows the given view controller by either displaying it in the `.secondary`
172+
/// column (split view) or pushing to the navigation stack.
173+
private func show(_ viewController: UIViewController, isLargeTitle: Bool = false) {
173174
if let splitViewController {
174175
let navigationVC = UINavigationController(rootViewController: viewController)
175176
if isLargeTitle {
@@ -182,6 +183,18 @@ final class ReaderPresenter: NSObject, SplitViewDisplayable {
182183
}
183184
}
184185

186+
/// Pushes the view controller to either the existing navigation stack in
187+
/// the `.secondary` column (split view) or to the main navigation stack.
188+
private func push(_ viewController: UIViewController) {
189+
if let splitViewController {
190+
let navigationVC = splitViewController.viewController(for: .secondary) as? UINavigationController
191+
wpAssert(navigationVC != nil)
192+
navigationVC?.pushViewController(viewController, animated: true)
193+
} else {
194+
mainNavigationController.pushViewController(viewController, animated: true)
195+
}
196+
}
197+
185198
private var splitViewController: UISplitViewController? {
186199
sidebar.splitViewController
187200
}
@@ -204,16 +217,16 @@ final class ReaderPresenter: NSObject, SplitViewDisplayable {
204217
viewModel.selection = .allSubscriptions
205218
case let .post(postID, siteID, isFeed):
206219
viewModel.selection = nil
207-
showSecondary(ReaderDetailViewController.controllerWithPostID(NSNumber(value: postID), siteID: NSNumber(value: siteID), isFeed: isFeed))
220+
show(ReaderDetailViewController.controllerWithPostID(NSNumber(value: postID), siteID: NSNumber(value: siteID), isFeed: isFeed))
208221
case let .postURL(url):
209222
viewModel.selection = nil
210-
showSecondary(ReaderDetailViewController.controllerWithPostURL(url))
223+
show(ReaderDetailViewController.controllerWithPostURL(url))
211224
case let .topic(topic):
212225
viewModel.selection = nil
213-
showSecondary(ReaderStreamViewController.controllerWithTopic(topic))
226+
show(ReaderStreamViewController.controllerWithTopic(topic))
214227
case let .tag(slug):
215228
viewModel.selection = nil
216-
showSecondary(ReaderStreamViewController.controllerWithTagSlug(slug))
229+
show(ReaderStreamViewController.controllerWithTagSlug(slug))
217230
}
218231
}
219232

‎WordPress/Classes/ViewRelated/Reader/ReaderSidebarSubscriptionsSection.swift‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,9 @@ struct ReaderSidebarSubscriptionsSection: View {
1313
private var subscriptions: FetchedResults<ReaderSiteTopic>
1414

1515
var body: some View {
16-
Label(Strings.allSubscriptions, systemImage: "checkmark.rectangle.stack")
16+
Label(Strings.subscriptions, systemImage: "checkmark.rectangle.stack")
1717
.tag(ReaderSidebarItem.allSubscriptions)
18+
.listItemTint(AppColor.brand)
1819

1920
ForEach(subscriptions, id: \.self) { site in
2021
Label {
@@ -44,5 +45,5 @@ struct ReaderSidebarSubscriptionsSection: View {
4445
}
4546

4647
private struct Strings {
47-
static let allSubscriptions = NSLocalizedString("reader.sidebar.allSubscriptions", value: "All Subscriptions", comment: "Reader sidebar button title")
48+
static let subscriptions = NSLocalizedString("reader.sidebar.button.subscriptions", value: "Subscriptions", comment: "Reader sidebar button title")
4849
}

‎WordPress/Classes/ViewRelated/Reader/ReaderSidebarTagsSection.swift‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,14 @@ struct ReaderSidebarTagsSection: View {
3737
} label: {
3838
Label(Strings.addTag, systemImage: "plus.circle")
3939
}
40+
.listItemTint(AppColor.brand)
4041

4142
Button {
4243
viewModel.navigate(.discoverTags)
4344
} label: {
4445
Label(Strings.discoverTags, systemImage: "sparkle.magnifyingglass")
4546
}
47+
.listItemTint(AppColor.brand)
4648
}
4749

4850
func delete(at offsets: IndexSet) {

‎WordPress/Classes/ViewRelated/Reader/ReaderSidebarViewController.swift‎

Lines changed: 74 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@ final class ReaderSidebarViewController: UIHostingController<AnyView> {
1616
let view = ReaderSidebarView(viewModel: viewModel)
1717
.environment(\.managedObjectContext, ContextManager.shared.mainContext)
1818
super.init(rootView: AnyView(view))
19+
20+
// TODO: (reader) fix on ipad
21+
title = Strings.reader
1922
}
2023

2124
required dynamic init?(coder aDecoder: NSCoder) {
@@ -47,33 +50,42 @@ private struct ReaderSidebarView: View {
4750
private var teams: FetchedResults<ReaderTeamTopic>
4851

4952
var body: some View {
50-
List(selection: $viewModel.selection) {
53+
list
54+
.toolbar {
55+
EditButton()
56+
}
57+
.tint(preferredTintColor)
58+
.accessibilityIdentifier("reader_sidebar")
59+
}
60+
61+
@ViewBuilder
62+
private var list: some View {
63+
let list = List(selection: $viewModel.selection) {
5164
// On iPhone, .sidebar style is rendered differently, so it
5265
// requires a bit more work to get the look we want.
53-
if viewModel.isCompactStyleEnabled {
54-
content.listRowBackground(Color.clear)
55-
.listRowSeparator(.hidden)
56-
.listRowInsets(EdgeInsets(top: 8, leading: 0, bottom: 8, trailing: 0))
66+
if viewModel.isCompact {
67+
content.listRowSeparator(.hidden)
5768
} else {
5869
content
5970
}
6071
}
61-
.listStyle(.sidebar)
62-
.navigationTitle(Strings.reader)
63-
.toolbar {
64-
EditButton()
72+
if viewModel.isCompact {
73+
list.listStyle(.plain).onAppear {
74+
viewModel.selection = nil // Remove the higlight
75+
}
76+
} else {
77+
list.listStyle(.sidebar)
6578
}
66-
.tint(preferredTintColor)
67-
.accessibilityIdentifier("reader_sidebar")
6879
}
6980

7081
@ViewBuilder
7182
private var content: some View {
7283
Section {
84+
let screens = ReaderStaticScreen.allCases
7385
ForEach(ReaderStaticScreen.allCases) {
74-
Label($0.localizedTitle, systemImage: $0.systemImage)
86+
makePrimaryNavigationItem($0.localizedTitle, systemImage: $0.systemImage)
7587
.tag(ReaderSidebarItem.main($0))
76-
.lineLimit(1)
88+
.listRowSeparator((viewModel.isCompact && $0 != screens.last) ? .visible : .hidden, edges: .bottom)
7789
.accessibilityIdentifier($0.accessibilityIdentifier)
7890
}
7991
}
@@ -93,6 +105,19 @@ private struct ReaderSidebarView: View {
93105
}
94106
}
95107

108+
private func makePrimaryNavigationItem(_ title: String, systemImage: String) -> some View {
109+
HStack {
110+
Label(title, systemImage: systemImage)
111+
.lineLimit(1)
112+
if viewModel.isCompact {
113+
Spacer()
114+
Image(systemName: "chevron.right")
115+
.font(.system(size: 14).weight(.medium))
116+
.foregroundStyle(.secondary.opacity(0.8))
117+
}
118+
}
119+
}
120+
96121
private var preferredTintColor: Color {
97122
if #available(iOS 18, *) {
98123
return AppColor.tint
@@ -105,10 +130,41 @@ private struct ReaderSidebarView: View {
105130
}
106131
}
107132

108-
@ViewBuilder
109-
private func makeSection<Content: View>(_ title: String, isExpanded: Binding<Bool>, @ViewBuilder content: () -> Content) -> some View {
110-
if #available(iOS 17, *) {
111-
Section(title, isExpanded: isExpanded) {
133+
private func makeSection<Content: View>(_ title: String, isExpanded: Binding<Bool>, @ViewBuilder content: @escaping () -> Content) -> some View {
134+
ReaderSidebarSection(title: title, isExpanded: isExpanded, isCompact: viewModel.isCompact, content: content)
135+
}
136+
}
137+
138+
private struct ReaderSidebarSection<Content: View>: View {
139+
let title: String
140+
@Binding var isExpanded: Bool
141+
var isCompact: Bool
142+
@ViewBuilder var content: () -> Content
143+
144+
var body: some View {
145+
if isCompact {
146+
Button {
147+
isExpanded.toggle()
148+
} label: {
149+
HStack {
150+
Text(title)
151+
.font(.subheadline.weight(.semibold))
152+
.foregroundStyle(.secondary)
153+
Spacer()
154+
Image(systemName: "chevron.down")
155+
.font(.system(size: 14).weight(.semibold))
156+
.foregroundStyle(AppColor.brand)
157+
}
158+
.contentShape(Rectangle())
159+
}
160+
.buttonStyle(.plain)
161+
.listRowInsets(EdgeInsets(top: 24, leading: 20, bottom: 6, trailing: 16))
162+
163+
if isExpanded {
164+
content()
165+
}
166+
} else if #available(iOS 17, *) {
167+
Section(title, isExpanded: $isExpanded) {
112168
content()
113169
}
114170
} else {
@@ -121,7 +177,7 @@ private struct ReaderSidebarView: View {
121177

122178
private struct Strings {
123179
static let reader = NSLocalizedString("reader.sidebar.navigationTitle", value: "Reader", comment: "Reader sidebar title")
124-
static let subscriptions = NSLocalizedString("reader.sidebar.section.subscriptions.tTitle", value: "Subscriptions", comment: "Reader sidebar section title")
180+
static let subscriptions = NSLocalizedString("reader.sidebar.section.subscriptions.title", value: "Subscriptions", comment: "Reader sidebar section title")
125181
static let lists = NSLocalizedString("reader.sidebar.section.lists.title", value: "Lists", comment: "Reader sidebar section title")
126182
static let tags = NSLocalizedString("reader.sidebar.section.tags.title", value: "Tags", comment: "Reader sidebar section title")
127183
static let organization = NSLocalizedString("reader.sidebar.section.organization.title", value: "Organization", comment: "Reader sidebar section title")

‎WordPress/Classes/ViewRelated/Reader/ReaderSidebarViewModel.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ final class ReaderSidebarViewModel: ObservableObject {
1111
private let contextManager: CoreDataStackSwift
1212
private var previousReloadTimestamp: Date?
1313

14-
@Published var isCompactStyleEnabled = false
14+
@Published var isCompact = false
1515

1616
var navigate: (ReaderSidebarNavigation) -> Void = { _ in }
1717

@@ -89,7 +89,7 @@ enum ReaderStaticScreen: String, CaseIterable, Identifiable, Hashable {
8989

9090
var systemImage: String {
9191
switch self {
92-
case .recent: "newspaper"
92+
case .recent: "checkmark.circle"
9393
case .discover: "safari"
9494
case .saved: "bookmark"
9595
case .likes: "star"

‎WordPress/Classes/ViewRelated/Reader/ReaderStreamViewController.swift‎

Lines changed: 5 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,6 @@ import AutomatticTracks
4545

4646
weak var navigationMenuDelegate: ReaderNavigationMenuDelegate?
4747

48-
var jetpackBannerView: JetpackBannerView?
49-
5048
private var syncHelpers: [ReaderAbstractTopic: WPContentSyncHelper] = [:]
5149

5250
private var syncHelper: WPContentSyncHelper? {
@@ -358,7 +356,7 @@ import AutomatticTracks
358356
NotificationCenter.default.addObserver(self, selector: #selector(postSeenToggled(_:)), name: .ReaderPostSeenToggled, object: nil)
359357

360358
configureCloseButtonIfNeeded()
361-
setupStackView()
359+
setupTableView()
362360
setupFooterView()
363361
setupContentHandler()
364362
setupResultsStatusView()
@@ -511,50 +509,12 @@ import AutomatticTracks
511509

512510
// MARK: - Setup
513511

514-
private func setupStackView() {
515-
let stackView = UIStackView()
516-
stackView.axis = .vertical
517-
stackView.translatesAutoresizingMaskIntoConstraints = false
518-
519-
setupTableView(stackView: stackView)
520-
setupJetpackBanner(stackView: stackView)
521-
522-
view.addSubview(stackView)
523-
NSLayoutConstraint.activate([
524-
stackView.topAnchor.constraint(equalTo: view.safeAreaLayoutGuide.topAnchor),
525-
stackView.leadingAnchor.constraint(equalTo: view.leadingAnchor),
526-
stackView.trailingAnchor.constraint(equalTo: view.trailingAnchor),
527-
stackView.bottomAnchor.constraint(equalTo: view.bottomAnchor)
528-
])
529-
}
530-
531-
private func setupJetpackBanner(stackView: UIStackView) {
532-
/// If being presented in a modal, don't show a Jetpack banner
533-
if let nav = navigationController, nav.isModal() {
534-
return
535-
}
536-
537-
guard JetpackBrandingVisibility.all.enabled else {
538-
return
539-
}
540-
let textProvider = JetpackBrandingTextProvider(screen: JetpackBannerScreen.reader)
541-
let bannerView = JetpackBannerView()
542-
bannerView.configure(title: textProvider.brandingText()) { [weak self] in
543-
guard let self else {
544-
return
545-
}
546-
JetpackBrandingCoordinator.presentOverlay(from: self)
547-
JetpackBrandingAnalyticsHelper.trackJetpackPoweredBannerTapped(screen: .reader)
548-
}
549-
jetpackBannerView = bannerView
550-
addTranslationObserver(bannerView)
551-
stackView.addArrangedSubview(bannerView)
552-
}
553-
554-
private func setupTableView(stackView: UIStackView) {
512+
private func setupTableView() {
555513
configureRefreshControl()
556514

557-
stackView.addArrangedSubview(tableViewController.view)
515+
view.addSubview(tableViewController.view)
516+
tableViewController.view.translatesAutoresizingMaskIntoConstraints = false
517+
view.pinSubviewToSafeArea(tableViewController.view)
558518
tableViewController.didMove(toParent: self)
559519
tableConfiguration.setup(tableView)
560520
tableView.delegate = self

‎WordPress/Classes/ViewRelated/Reader/Subscriptions/ReaderSubscriptionAddButton.swift‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,12 @@ struct ReaderSubscriptionAddButton: View {
1212

1313
var body: some View {
1414
button.popover(isPresented: $isShowingPopover) {
15-
ReaderSubscriptionAddView()
15+
if #available(iOS 16.4, *) {
16+
ReaderSubscriptionAddView()
17+
.presentationCompactAdaptation(.popover)
18+
} else {
19+
ReaderSubscriptionAddView()
20+
}
1621
}
1722
}
1823

@@ -62,7 +67,7 @@ private struct ReaderSubscriptionAddView: View {
6267
.onChange(of: siteURL) { _ in
6368
displayedError = nil
6469
}
65-
.frame(width: 420)
70+
.frame(idealWidth: 420)
6671
.interactiveDismissDisabled(!siteURL.isEmpty)
6772
}
6873

0 commit comments

Comments
 (0)