Skip to content
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.

Sign up for GitHub

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

Fix an issue with posts shown embedded in the notifications popover #23889

Merged
merged 2 commits into from
Dec 13, 2024
Merged

Fix an issue with posts shown embedded in the notifications popover #23889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa6e618
Fix an issue with posts shown embedded in the notifications popover
kean Dec 12, 2024
b0ecab1
Update release notes
kean Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
* [*] Add support for restricted posts in Reader [#23853]
* [*] Fix minor appearance issues in the Blaze campaign list [#23891]
* [*] Improve the sidebar animations and layout on some iPad models [#23886]
* [*] Fix an issue with posts shown embedded in the notifications popover on iPad [#23889]

25.5
-----
Expand Down
6 changes: 2 additions & 4 deletions WordPress/Classes/System/Root View/ReaderPresenter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,11 +230,9 @@ final class ReaderPresenter: NSObject, SplitViewDisplayable {
case .subscriptions:
viewModel.selection = .allSubscriptions
case let .post(postID, siteID, isFeed):
viewModel.selection = nil
show(ReaderDetailViewController.controllerWithPostID(NSNumber(value: postID), siteID: NSNumber(value: siteID), isFeed: isFeed))
push(ReaderDetailViewController.controllerWithPostID(NSNumber(value: postID), siteID: NSNumber(value: siteID), isFeed: isFeed))
case let .postURL(url):
viewModel.selection = nil
show(ReaderDetailViewController.controllerWithPostURL(url))
push(ReaderDetailViewController.controllerWithPostURL(url))
case let .topic(topic):
viewModel.selection = nil
show(ReaderStreamViewController.controllerWithTopic(topic))
Expand Down
16 changes: 11 additions & 5 deletions ...d/Notifications/Controllers/NotificationsViewController/NotificationsViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,11 +777,17 @@ extension NotificationsViewController {
if let postID = note.metaPostID,
let siteID = note.metaSiteID,
note.kind == .matcher || note.kind == .newPost {
let readerViewController = ReaderDetailViewController.controllerWithPostID(postID, siteID: siteID)
readerViewController.navigationItem.largeTitleDisplayMode = .never
readerViewController.hidesBottomBarWhenPushed = true
readerViewController.coordinator?.notificationID = note.notificationId
displayViewController(readerViewController)

if isSidebarModeEnabled && splitViewController == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a roundabout way to check if the notifications view controller is displayed in a popover.

Maybe it's more intuitive to check whether self.modalPresentationStyle or self.navigationController.modalPresentationStyle is popover?

Copy link
Contributor Author

@kean kean Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but after a quick search I couldn't find a reliable way to check if something is displayed in a popover. I guess the modalPresentationStyle check could work in this case. I'm a bit reluctant to use it because it's only not a completely reliable indicator as on iPhone .popover is displayed in a sheet by default (obviously not the case here for this screen).

Btw, I'll try to remove isSidebarModeEnabled as well during these couple of weeks. It was added there as a temporary measure during the iPad rework and is a type of a feature flag.

presentingViewController?.dismiss(animated: true)
RootViewCoordinator.sharedPresenter.showReader(path: .post(postID: postID.intValue, siteID: siteID.intValue))
} else {
let readerViewController = ReaderDetailViewController.controllerWithPostID(postID, siteID: siteID)
readerViewController.navigationItem.largeTitleDisplayMode = .never
readerViewController.hidesBottomBarWhenPushed = true
readerViewController.coordinator?.notificationID = note.notificationId
displayViewController(readerViewController)
}
return
}

Expand Down