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

CPT: Display post author in custom post types list #6686

Merged
merged 2 commits into from
Jul 17, 2016
Merged

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Jul 11, 2016

This pull request seeks to add the post's author to the meta information on the custom post types list screen. It also enables changing the author inline, making use of the existing <AuthorSelector /> component (used also in the post editor). (Removed per feedback)

Before After
Before After

See original concept

Original version with author selector

Testing instructions:

Verify that you are able to view the author of a post, and reassign the author if you have capabilities for doing so.

  1. Navigate to the custom post types list screen
  2. Select a site, if prompted
  3. Select a filter where posts exist
  4. Note that the author is shown~~, with Gravatar,~~ under the post title
  5. If capabilities exist, note that a chevron is shown adjacent the author to select a new author
  6. Select a new author
  7. Note that after a brief delay, the author is changed and persists through page refresh

Caveats / Follow-up Tasks:

These caveats are no longer applicable after removal of author selector per feedback.

  • The change in author is not reflected instantaneously, because the format of author is different between reading and updating endpoints. This should be resolved by normalizing the post object's author in the getNormalizedPost selector.
  • The author selector does not account for the capabilities of the author to which the post will be assigned. Even if a user does not have capabilities of editing a particular post type, it will still be presented as an option in the dropdown. This is an issue of the underlying <AuthorSelector /> component.

Test live: https://calypso.live/?branch=add/cpt-assign-author

@aduth aduth added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR Custom Post Types (CPT) labels Jul 11, 2016
@folletto
Copy link
Contributor

I'm not sure if we should make the author changeable from here, especially since it won't have a confirmation.

That said, if we really think we want to have the ability to change it in there, I feel that visually should match the original designs:

screen shot 2016-07-11 at 20 07 55

@hugobaeta
Copy link

@folletto and I were just discussing this, I agree with his comment: I feel unnecessary to provide that functionality in the card - this is meant to be a quick overview, before the user takes the primary action of editing the entry.

The avatar, even tho it's a logical addition, feels too heavy in that meta information list - specially in the second position, it adds a visual weight to the middle of the card that takes attention away from the intended hierarchy in the card). Also, at such small size, it becomes a blur :) Let's ditch it and use the icon I used in the mockup, please.

@nylen
Copy link
Contributor

nylen commented Jul 11, 2016

I also think providing the ability to change authorship here is more confusing than helpful. Changing the author of a post is a very uncommon operation (0.08% of editor_actions stat).

@aduth
Copy link
Contributor Author

aduth commented Jul 12, 2016

Thanks for the feedback. I removed the option to reassign the author and the author's Gravatar.

Updated screenshot:

image

@folletto
Copy link
Contributor

folletto commented Jul 12, 2016

Checked on Chrome, design wise it's 👍

(We should remove the "(last updated)" bit, but can be done in a separate PR, no need here)

@aduth
Copy link
Contributor Author

aduth commented Jul 12, 2016

(We should remove the "(last updated)" bit, but can be done in a separate PR, no need here)

@hugobaeta mentioned this once before, and it's always been part of the underlying <PostRelativeTimeStatus /> component for draft posts (reference), originally from the posts screen. Since we effectively did away with a dedicated post drafts screen, I think we could remove this without it affecting anything else?

@folletto
Copy link
Contributor

folletto commented Jul 12, 2016

Yeah. I think we can get away from that, we might replace later with a tooltip, but the date is the latest update (or, publish in case of published posts) so it should be contexually clear what that is. :)

@aduth
Copy link
Contributor Author

aduth commented Jul 12, 2016

See #6707 for removal of last modified label.

@aduth aduth changed the title CPT: Display assignable author selector in list post CPT: Display post author in custom post types list Jul 12, 2016
@nylen
Copy link
Contributor

nylen commented Jul 13, 2016

Looks good & works well. Could use a rebase but otherwise good to go.

@nylen nylen added [Status] Ready to Merge and removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 13, 2016
@aduth aduth force-pushed the add/cpt-assign-author branch from fdff9a6 to 5c3cd19 Compare July 16, 2016 23:57
@aduth aduth merged commit 2161736 into master Jul 17, 2016
@aduth aduth deleted the add/cpt-assign-author branch July 17, 2016 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants