-
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
Posts & Pages: Move post actions to a context menu #21850
Posts & Pages: Move post actions to a context menu #21850
Conversation
switch self { | ||
case .retry: UIImage() | ||
case .view: UIImage(systemName: "eye") | ||
case .publish: UIImage(systemName: "icon.globe") |
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.
- There is no
icon.globe
but there isglobe
- I'd suggest using
chart.bar
because it has no fill
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.
Done! 11ab0be
buttons.append(.stats) | ||
} | ||
|
||
// TODO: Add reader and comments |
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.
Reader is out of the scope p1696870367221659/1696868289.514809-slack-C04SFL0RP51 (no need to update the comment)
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.
Ah good shout, thanks for the reminder!
@@ -2,5 +2,4 @@ import Foundation | |||
|
|||
protocol InteractivePostView { | |||
func setInteractionDelegate(_ delegate: InteractivePostViewDelegate) |
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.
I hope I'm going to able to use PostListViewController
implementation of the delegate in the search list. I think hit should work, but If not, I'll extract it to a separate class.
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.
I think we'll end up having to extract it to a separate class, if we want to reuse it for pages. The pages screen doesn't use the delegate - it builds the action sheet straight in the class 😞
|
||
return buttons.map { button in | ||
UIAction(title: button.title(for: post), image: button.icon, attributes: button.attributes, handler: { _ in | ||
button.performAction(for: post, view: presentingView, delegate: delegate) |
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.
Not sure if there is a retain cycle, but I would suggest making the delegate weak
just in case.
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.
Done! 67b8085
I think Chris wanted us to start adopting the Gutenberg icons. It can be done separately. It's not always easy to find what you need there. |
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.
Approving, but with a couple of comments.
} | ||
buttons.append(.share) | ||
} | ||
if autoUploadInteractor.canRetryUpload(of: post) || |
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.
I would suggest removing isInternetReachable
. It's unreliable.
private typealias StatusMessages = PostCardStatusViewModel.StatusMessages | ||
|
||
class PostCardCellTests: CoreDataTestCase { | ||
private var postCell: PostCardCell! |
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.
Is there a plan to re-implement (some of) these tests?
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.
Yes! I'll add them in the next PR
Generated by 🚫 dangerJS |
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
Closes #21717
Description
Notes
How to test
Regression Notes
Potential unintended areas of impact
Post actions
What I did to test those areas of impact (or what existing automated tests I relied on)
Tested manually
What automated tests I added (or what prevented me from doing so)
n/a
PR submission checklist:
RELEASE-NOTES.txt
if necessary.UI Changes testing checklist: