-
Notifications
You must be signed in to change notification settings - Fork 71
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
Allow bulk article deletions from Apple News list table #1207
Allow bulk article deletions from Apple News list table #1207
Conversation
This allows the article to be republished. Otherwise, it will appear as though the article is already 'in sync' with Apple News.
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.
a couple small things, otherwise 👍
assets/js/bulk-export.js
Outdated
searchParams = new URLSearchParams( window.location.search ), | ||
$submitButton = $( '.bulk-export-submit' ); | ||
|
||
function done() { | ||
$( '.bulk-export-submit' ).text( 'Done' ); | ||
$submitButton.text( 'Done' ); | ||
$submitButton.attr( 'disabled', 'disabled' ); |
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.
weird indentation through here?
$apple_page_description ??= __( 'The following articles will be affected.', 'apple-news' ); | ||
$apple_posts ??= []; | ||
$apple_submit_text ??= __( 'Go', 'apple-news' ); |
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 old hard-coded text tells me that "Bulk Export" will publish the posts to Apple News. This new text does not tell me that. If I saw "Bulk Export", I would expect it to download a file vs publishing. Maybe "Bulk Export" needs to be "Bulk Publish"?
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 current language was retained and is supposed to be applied here:
apple-news/admin/class-admin-apple-bulk-export-page.php
Lines 86 to 115 in 83b8bec
// Allow only specific actions. | |
if ( ! in_array( $action, [ 'apple_news_push_post', 'apple_news_delete_post' ], true ) ) { | |
wp_die( esc_html__( 'Invalid action.', 'apple-news' ), '', [ 'response' => 400 ] ); | |
} | |
// Populate articles array with a set of valid posts. | |
$apple_posts = []; | |
foreach ( explode( ',', $post_ids ) as $post_id ) { | |
$post = get_post( (int) $post_id ); | |
if ( ! empty( $post ) ) { | |
$apple_posts[] = $post; | |
} | |
} | |
// Override text within the partial depending on the action. | |
$apple_page_title = match ( $action ) { | |
'apple_news_push_post' => __( 'Bulk Export Articles', 'apple-news' ), | |
'apple_news_delete_post' => __( 'Bulk Delete Articles', 'apple-news' ), | |
}; | |
$apple_page_description = match ( $action ) { | |
'apple_news_push_post' => __( 'The following articles will be exported.', 'apple-news' ), | |
'apple_news_delete_post' => __( 'The following articles will be deleted.', 'apple-news' ), | |
}; | |
$apple_submit_text = match ( $action ) { | |
'apple_news_push_post' => __( 'Export', 'apple-news' ), | |
'apple_news_delete_post' => __( 'Delete', 'apple-news' ), | |
}; | |
require_once __DIR__ . '/partials/page-bulk-export.php'; |
With the changes in this PR, this partial should only ever be rendered when a known bulk action is being invoked, and the text should reflect that action (which happens in the lines linked above).
There isn't any circumstance where the default strings seen here should actually be rendered, but I included them so that there would be something on the page if it ever happened. You're right that the strings are vague, but how the page would behave at that point is also not known, so I wasn't sure what else to do.
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.
cool, I was kind of picking that up from the code. this makes sense. I still wonder about using Export
vs Upload
or Publish
, but maybe that's a question for project maintainers or stakeholders. The button text was Publish All
and I don't see that it is retained, but maybe I'm missing it. Anyway, I'm good with whatever, so feel free to proceed as you see fit.
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.
Thanks for catching that the button text wasn't actually kept! Whoops. Fixed in 0ab4d9c.
Re. the word Export
, I agree that it isn't as clear as it could be. I'm reluctant to change it here, but, as it happens, @scottnelle has been working on refining that language in #1204 (see also https://l.alley.dev/a3a948a289), so maybe we could expand that PR to include language for this page that will be consistent with the change being made on the list table.
Fixes #1205.
Fixes #1172.
Fixes #1206.