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

Fix: Publishing UX for contributors #6729

Merged
merged 10 commits into from Jun 19, 2018

Conversation

@nfmohit-wpmudev
Contributor

nfmohit-wpmudev commented May 13, 2018

Description

This PR addresses #6725 which reports the incorrect post publishing UX for contributors who doesn't have publishing capability.

How has this been tested?

This PR was tested by verifying the following:

  • Ensuring that the contributor only ever sees a "Submit for Review" button.
  • The "Are you ready to publish?" copy is updated to reflect the submitting for review workflow.
  • The invalid visibility and scheduling UI is removed.

This was tested in WP 4.9.5, Gutenberg 2.8.0, Apache server with PHP 7.2.0 and MySQL 5.6.34. According to initial tests, the code doesn’t seem to affect any other areas.

Screenshots

As an administrator:
gutenberg-6725-admin

As a contributor:
gutenberg-6725-contributor

Types of changes

For the toggle button, this just enforces a condition to check if the user is a contributor or not with the isContributor constant variable and then display the correct label. For the visibility and scheduling UI along with the copy above it, they have been changed/removed depending on the user being a contributor or not using the same isContributor constant variable.

Note

This PR will be amended later to use wp:action-publish and hasPublishAction as soon as the related #6724 gets merged.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo requested a review from danielbachhuber May 14, 2018

@gziolo gziolo added this to the 2.9 milestone May 14, 2018

function PostPublishPanelPrepublish( {
user,
} ) {
const userCanPublishPosts = get( user.data, [ 'post_type_capabilities', 'publish_posts' ], false );

This comment has been minimized.

@danielbachhuber

danielbachhuber May 14, 2018

Member

Can you incorporate #6724 into this pull request?

This comment has been minimized.

@nfmohit-wpmudev

nfmohit-wpmudev May 15, 2018

Contributor

Incorporated #6724 into this PR in this commit.

Thank you so much again for reviewing this @danielbachhuber.

@@ -42,6 +42,21 @@ function PostPublishPanelToggle( {
return <PostPublishButton forceIsDirty={ forceIsDirty } forceIsSaving={ forceIsSaving } />;
}
if ( isContributor ) {

This comment has been minimized.

@danielbachhuber

danielbachhuber May 14, 2018

Member

Now that we have some conditional logic in this file, would it be worthwhile to add a couple of unit tests?

Check out post-publish-button for the pattern to follow.

This comment has been minimized.

@nfmohit-wpmudev

nfmohit-wpmudev May 15, 2018

Contributor

Absolutely! I believe so.

I'm still working on it, taking some time as I never added unit tests previously. Looking into the pattern of post-publish-button for reference.

Thank you @danielbachhuber.

This comment has been minimized.

@nfmohit-wpmudev

nfmohit-wpmudev May 17, 2018

Contributor

Hey @danielbachhuber!

The conditional logics have been changed in this file as per this commit according to this recommendation. Do you think this still needs unit tests?

If it does, could you advice a bit regarding how I could approach to achieve that with an example if possible in the updated conditional logics? I have been checking out other components along with post-publish-button and also this documentation. Still, this is me first time adding unit tests so having a bit hiccups 😅.

Thank you so much ❤

This comment has been minimized.

@danielbachhuber

danielbachhuber May 17, 2018

Member

Do you think this still needs unit tests?

Yes, I think it would be ideal to have tests.

If it does, could you advice a bit regarding how I could approach to achieve that with an example if possible in the updated conditional logics?

I'm about to be offline for a bit so not immediately. Want to try a first pass by copy and pasting out of post-publish-button, and then I'll follow up later on to make any necessary changes? Feel free to stop by #core-editor if you need help; I'm sure there's someone around that can help get you started.

This comment has been minimized.

@nfmohit-wpmudev

nfmohit-wpmudev May 18, 2018

Contributor

Thank you @danielbachhuber !

Just pushed a new commit with the unit tests. I'm quite sure it's all wrong, looking forward to getting your help in it if possible. Thank you so much.

This comment has been minimized.

@danielbachhuber

danielbachhuber May 18, 2018

Member

Looks great! I added a few more tests in 625fcd9

This comment has been minimized.

@nfmohit-wpmudev

nfmohit-wpmudev May 18, 2018

Contributor

Awesome one 😍, thank you so much! ❤

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 14, 2018

Thanks for the PR, @nfmohit! Couple of points to address.

@nfmohit-wpmudev

This comment has been minimized.

Contributor

nfmohit-wpmudev commented May 15, 2018

Thank you for reviewing @danielbachhuber <3 Working on the changes.

@@ -33,4 +51,13 @@ function PostPublishPanelPrepublish() {
);
}
export default PostPublishPanelPrepublish;
export default compose( [

This comment has been minimized.

@youknowriad

youknowriad May 16, 2018

Contributor

There's only a single HoC used here, we can safely remove compose

<p>{ __( 'Here, you can do a last-minute check up of your settings below, before you submit for review.' ) }</p>
</div>
);
}

This comment has been minimized.

@youknowriad

youknowriad May 16, 2018

Contributor

It feels that a lot of things are shared with the code bellow. Maybe we can just do something like:

{ hasPublishAction ? __( 'Are you ready to publish?' ) : __( 'Are you ready to submit for review?' ) }

And wrap the panels inside a

{ hasPublishAction && (
<Fragment>
  // panels go here
</Fragment>
) }
disabled={ ! isButtonEnabled }
isBusy={ isSaving && isPublished }
>
{ isBeingScheduled ? __( 'Schedule…' ) : __( 'Submit for Review…' ) }

This comment has been minimized.

@youknowriad

youknowriad May 16, 2018

Contributor

Same here, it looks like the only difference is the label, so the check should be done inside the button.

Also, I remember that it was an intended change to show "Publish" even for contributors. The idea is to keep the button small and wen you open the panel, you'll see the right message. cc @jasmussen

This comment has been minimized.

@jasmussen

jasmussen May 16, 2018

Contributor

Correct. The context is largely in #5343, but the idea is that submitting for review is definitely part of the publishing flow.

Scheduling is also part of the publishing flow, but it could be argued that if you've already chosen a different scheduling time in the Settings sidebar before starting the publishing flow, it should say "Schedule..."

However this is a sensitive change that needs to be properly tested on a 4 inch iphone with longer languages, such as German.

@youknowriad youknowriad modified the milestones: 2.9, 3.0 May 16, 2018

@youknowriad

Code wise, this looks good to me, just waiting for a confirmation from the design side of things cc #6729 (comment)

) }
{ hasPublishAction && (
isBeingScheduled ? __( 'Schedule…' ) : __( 'Publish…' )
) }

This comment has been minimized.

@youknowriad

youknowriad May 16, 2018

Contributor

Minor: Do you think if would be clearer if we avoid ternaries here:

{ isBeingScheduled && __( 'Schedule…' ) }
{ ! isBeingScheduled &&  hasPublishAction && __( 'Publish…' ) }
{ ! isBeingScheduled && ! hasPublishAction && __( 'Submit for Review…' ) }

This comment has been minimized.

@nfmohit-wpmudev

nfmohit-wpmudev May 16, 2018

Contributor

Thank you so much for reviewing this @youknowriad!

This looks way clearer to me. Test in progress and another change incoming...

This comment has been minimized.

@nfmohit-wpmudev

nfmohit-wpmudev May 16, 2018

Contributor

Done here.

I had initially tried to nest ternaries but Travis didn't like it. But this looks way nicer. Thank you for the recommendation @youknowriad.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 18, 2018

Code is looking fine now. For Needs Design Feedback, we'll need to sort out this:

image

I'm thinking that maybe we shouldn't display the pre-publish panel for contributors, unless someone has a good idea for what should go in it.

@nfmohit-wpmudev

This comment has been minimized.

Contributor

nfmohit-wpmudev commented May 19, 2018

Thank you @danielbachhuber

I'm thinking that maybe we shouldn't display the pre-publish panel for contributors

I can work on this if everyone prefers, but as per the comments here and here, @youknowriad and @jasmussen recommends that we make the publish toggle button say "Publish..." for contributors as well. In that case, wouldn't the pre-publish-panel be required for contributors?

@tofumatt

This comment has been minimized.

Member

tofumatt commented May 28, 2018

Looks like @karmatosed agrees (she left a 👍 reaction to your question) but I don't think GitHub sends notifications about reactions so it might have been unclear.

Sounds like the pre-publish-panel is required for contributors and we should be displaying it.

To @danielbachhuber's question, what should we put in the panel? (cc @karmatosed)

@youknowriad youknowriad modified the milestones: 3.0, 3.1 May 29, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented May 30, 2018

I'm thinking that maybe we shouldn't display the pre-publish panel for contributors, unless someone has a good idea for what should go in it.

I think we should keep it, but that we should rephrase the text, and consider the contributor role for the pre-publish plugin API.

I understand your concern — if there's nothing for a contributor to change there, why have a publish screen at all?

Perhaps the most important aspect of blogging is that there is friction, just the right amount, to publishing. The "Publish" button is a last moment to reflect on what you wrote, and a celebratory moment when pressed. It is also a better interface than an "Are you sure?" confirm.

I can work on this if everyone prefers, but as per the comments here and here, @youknowriad and @jasmussen recommends that we make the publish toggle button say "Publish..." for contributors as well. In that case, wouldn't the pre-publish-panel be required for contributors?

Yes. Even as a contributor, you are doing a publishing action by submitting for review. You are publishing to your editor to approve.

The other problem this solves, is ensuring that there is space for the button on a mobile device, where "Submit for Review..." would not fit on an iPhone 5 with the other minimal UI in the toolbar, and in other languages it would be worse.

So, I think this is almost ready to ship. Let's change the "Submit for Review..." to "Publish...", and inside the confirm panel, let's rephrase the text. It could for example read:

Are you ready to submit for review?
When you're ready, submit your work for review, and an Editor will be able to approve it for you.

That's bad, but I tagged "Needs Copy Review" in case someone has a better idea.

I can also imagine a separate pre-submit-for-review API that could show plugin slot fills in the area below. I imagine an SEO score would be nice to know before you submit for review.

@michelleweber

This comment has been minimized.

michelleweber commented May 30, 2018

I feel like there is the potential for confusion when "publish" language is used, because publish usually means that something is going live, so I'd like to try and help that with the language that displays in the panel -- maybe something like:

Ready to submit this for publication?
Double-check your settings, make any final changes, and click "publish" to submit your post -- an editor will review, approve, and publish it.

@nfmohit-wpmudev

This comment has been minimized.

Contributor

nfmohit-wpmudev commented May 30, 2018

Thank you for your valuable feedback over this @tofumatt and @jasmussen <3

In the last commit, I've restored the toggle button label from "Submit for Review…" to "Publish…" for contributors.

Thank you for your copy suggestion @michelleweber !

I've still went with @jasmussen's suggested copy for this commit for now, unless we have a more appropriate copy and confirmations over that.

@michelleweber's copy sounded good, but one thing to note here is, the contributors will be finally clicking the "Submit for Review" button in the post pre-publish panel as the post publish button still has the label of "Submit for Review" for contributors.

Thanks guys <3

expect( wrapper.childAt( 0 ).text() ).toBe( 'Schedule…' );
} );
it( 'should display Schedule… if able to be published', () => {

This comment has been minimized.

@nfmohit-wpmudev

nfmohit-wpmudev May 30, 2018

Contributor

@danielbachhuber Just asking, I'm not sure to be honest, shouldn't it be should display Publish… if able to be published instead of should display Schedule… if able to be published?

Let me know if it's not correct and if I should fix it.

This comment has been minimized.

@danielbachhuber

danielbachhuber May 30, 2018

Member

Hm. I think this should be:

should display Publish… if able to be published

This comment has been minimized.

@nfmohit-wpmudev

nfmohit-wpmudev May 30, 2018

Contributor

I assumed so, let me fix it. Thank you for looking <3

This comment has been minimized.

@nfmohit-wpmudev

nfmohit-wpmudev May 30, 2018

Contributor

Just did in this commit.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 19, 2018

@jasmussen Any remaining changes needed for this or can it be merged?

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 19, 2018

Actually, I see @jasmussen gave a 👍 to @nfmohit-wpmudev's most recent comment.

@nfmohit-wpmudev Thanks again for your work on this!

@danielbachhuber danielbachhuber merged commit 6bf15a3 into WordPress:master Jun 19, 2018

2 checks passed

codecov/project 46.46% (+0.03%) compared to 86a4b22
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 20, 2018

GIF looks like this:

mycontributorux

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 20, 2018

All good and thank you!

@nfmohit-wpmudev

This comment has been minimized.

Contributor

nfmohit-wpmudev commented Jun 20, 2018

Thank you guys! <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment