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.

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

SwG PoC: Bulk Edit #4150

Closed
wants to merge 23 commits into from
Closed

Conversation

ChrisAntaki
Copy link
Contributor

Summary

Addresses issue #

Relevant technical choices

Checklist

  • My code is tested and passes existing unit tests.
  • My code has an appropriate set of unit tests which all pass.
  • My code is backward-compatible with WordPress 4.7 and PHP 5.6.
  • My code follows the WordPress coding standards.
  • My code has proper inline documentation.
  • I have added a QA Brief on the issue linked above.
  • I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

@google-cla google-cla bot added the cla: yes label Sep 28, 2021
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Left some feedback. I think there's some room for simplification in the React to non-React JS "communication", but otherwise this looks great.

assets/js/googlesitekit-subscribe-with-google-bulk-edit.js Outdated Show resolved Hide resolved
assets/js/googlesitekit-subscribe-with-google-bulk-edit.js Outdated Show resolved Hide resolved
assets/js/googlesitekit-subscribe-with-google-bulk-edit.js Outdated Show resolved Hide resolved
assets/js/googlesitekit-subscribe-with-google-bulk-edit.js Outdated Show resolved Hide resolved
includes/Modules/Subscribe_With_Google.php Outdated Show resolved Hide resolved
includes/Modules/Subscribe_With_Google.php Outdated Show resolved Hide resolved
Comment on lines 93 to 114
add_filter(
'manage_post_posts_columns',
function( $columns ) {
return array_merge( $columns, array( 'sitekit__reader_revenue__access' => __( 'Access', 'google-site-kit' ) ) );
}
);

add_action(
'manage_post_posts_custom_column',
function( $column_key, $post_id ) {
if ( 'sitekit__reader_revenue__access' === $column_key ) {
$access = get_post_meta( $post_id, 'sitekit__reader_revenue__access', true );
if ( $access && 'openaccess' !== $access ) {
echo esc_html( $access );
} else {
esc_html_e( '— Free —', 'google-site-kit' );
}
}
},
10,
2
);
Copy link
Member

Choose a reason for hiding this comment

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

No technical concern with this code, but from a UX perspective I wonder if that is the most appropriate way to show the access information for a post in the table. A whole column takes a lot of space, and while users can hide columns in the UI, I think there may be more subtle ways that would also show this information closer to e.g. the post title. Especially since not every post is expected to have a special state (thinking about sites that would monetize let's say 5 out of 100 posts), I'm not sure this is worth a column.

For Idea Hub for example, we display something additional right in the post title column. Maybe that could be a path.

You probably don't need to change that here unless you feel strongly about that, but I think this may get flagged from a UX perspective eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't feel strongly either way, just wanna build something you and the Site Kit team are happy with

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants