-
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
Comment Detail Redesign: Moderation Bar Update #19331
Comment Detail Redesign: Moderation Bar Update #19331
Conversation
updateModerationBarVisibility() | ||
} | ||
} | ||
// var hidesModerationBar: Bool = false { |
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.
Commented out snippets are addressed and will be included in next PR's.
case .content: | ||
return nil | ||
case .moderation: | ||
return "STATUS" |
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.
This is localized and will be updated in next PR's.
You can test the changes in Jetpack from this Pull Request by:
|
You can test the changes in WordPress from this Pull Request by:
|
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.
One minor thing I noticed is if you select the same status that is already selected, it flashes. Probably from a reload would be my guess.
Other than that, LGTM!
spamComment() | ||
case .unapproved: | ||
trashComment() | ||
return NSLocalizedString("Spam", comment: "Button title for Spam comment state.") |
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 thought there was a P2 somewhere saying we needed to start adding keys to localized strings but all I could find was paNNhX-nP-p2. It doesn't say we need keys, just that the option is there.
Regardless, I don't think it's a huge deal since I can't find the post anywhere. Just wanted to point it out in case you happen to know more.
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.
Oh no I saw that as well and have been doing it. These are existing strings but I can do it regardless
@wargcm That blinking issue is resolved in my local branch but I added a condition to not allow a selected status to be selected regardless. So it should fix it here as well. |
Fixes #18999
This PR removes the old moderation bar from comments section and adds a new table-view version of it.
To test:
Regression Notes
Potential unintended areas of impact
Comments' status may not function properly
What I did to test those areas of impact (or what existing automated tests I relied on)
Manually tested it. Follow the steps above.,
What automated tests I added (or what prevented me from doing so)
N/A
PR submission checklist:
RELEASE-NOTES.txt
if necessary.