-
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
[Notification Refresh] Replace the old Comment Detail header with the new Content Preview component #23083
[Notification Refresh] Replace the old Comment Detail header with the new Content Preview component #23083
Conversation
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr23083-e8a5b25 | |
Version | 24.7 | |
Bundle ID | org.wordpress.alpha | |
Commit | e8a5b25 | |
App Center Build | WPiOS - One-Offs #9713 |
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr23083-e8a5b25 | |
Version | 24.7 | |
Bundle ID | com.jetpack.alpha | |
Commit | e8a5b25 | |
App Center Build | jetpack-installable-builds #8757 |
30afe1e
to
f15f3b2
Compare
697f706
to
cba89b4
Compare
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.
Great job, Salim! 🎸
I tested it on different notifications and found an unusual case. There is a case when a comment is opened by ReaderCommentsViewController
on top of CommentDetailViewController
. I assume it happens when your original comment was posted to a blog you don't have moderation permissions.
As a result such a notification has the old design:
In addition, I opened a PR when replaced the navigation icons: #23107
Feel free to create a follow up PR with this change if you wish.
Modules/Sources/DesignSystem/Components/Content Preview/ContentPreview.swift
Show resolved
Hide resolved
This behavior was developed in the 1st phase where we redirect the user directly to Reader Comments screen if they don't permission to moderate the comment. Also based on the View Debugger screenshot you shared, the Reader Comments screen is a child of As for old header design in the Reader Comments, I intentionally avoided redesigning it because I think it's out of scope of this phase. 🤔 |
Yeah, totally understand it, but the screen looks like a notification detail and does even have navigation buttons on the toolbar. That's why it looks confusing and not organically within the brand-new design system. 🤔 |
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.
The PR looks and works good to me. We can discuss the issue from above asynchronously.
2d235b3
into
feature/notifications_refresh_p2
Fixes https://github.com/Automattic/wordpress-mobile/issues/29
Test Instructions
Post Reply Notification
Comment Reply Notification
Regression Notes
Potential unintended areas of impact
The behavior when the header is tapped should be maintained.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing
What automated tests I added (or what prevented me from doing so)
None.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: