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

Chrome: Do not show the publish panel when updating/scheduling/submitting a post #4157

Merged
merged 5 commits into from Jan 5, 2018

Conversation

Projects
None yet
5 participants
@youknowriad
Contributor

youknowriad commented Dec 25, 2017

This PR updates the publish panel behavior.
The panel is now only shown when "publishing" a post. It's not shown when we update an existing post, schedule a post or submit for review.

Related #3496 (comment)

Testing instructions

  • Create a new post
  • Click publish
  • The publish panel should show up
  • Publish the post
  • Edit the post
  • Click update
  • The publish panel should not show up
@noisysocks

Works great in my testing! 😄

Not related, but the Publish and Settings button in the Header don't quite align properly on mobile:

screen shot 2017-12-28 at 10 58 24

Also, maybe it's just me, but the 'Switch to Draft' button looks a little visually unbalanced in its new home next to the 'Move to trash' button:

screen shot 2017-12-28 at 10 30 49

Show outdated Hide outdated editor/components/post-publish-panel/toggle.js Outdated
const isContributor = user.data && ! userCanPublishPosts;
const showToggle = ! isContributor && ! isPublished && ! isBeingScheduled;
if ( ! showToggle ) {

This comment has been minimized.

@noisysocks

noisysocks Dec 28, 2017

Member

It feels a little weird to me that rendering one component (PostPublishPanelToggle) might render a completely different component (PostPublishButton).

Perhaps this logic should be hoisted out into Header or into a new wrapper component?

@noisysocks

noisysocks Dec 28, 2017

Member

It feels a little weird to me that rendering one component (PostPublishPanelToggle) might render a completely different component (PostPublishButton).

Perhaps this logic should be hoisted out into Header or into a new wrapper component?

This comment has been minimized.

@youknowriad

youknowriad Dec 28, 2017

Contributor

The idea is to avoid using the post selectors in the edit-post directory which should be concerned only about UI and should be moved to a separate module in the future.

But you're right UI and state are a bit mixed here but that's the "balance" I've found for now.
Maybe I should rename the component something like PublishToggleOrUpdate :)

@youknowriad

youknowriad Dec 28, 2017

Contributor

The idea is to avoid using the post selectors in the edit-post directory which should be concerned only about UI and should be moved to a separate module in the future.

But you're right UI and state are a bit mixed here but that's the "balance" I've found for now.
Maybe I should rename the component something like PublishToggleOrUpdate :)

This comment has been minimized.

@youknowriad

youknowriad Dec 28, 2017

Contributor

Once the data module is updated with state exposing capabilities, we could move some of this logic outside the components folder and still access exposed data from the editor module.

@youknowriad

youknowriad Dec 28, 2017

Contributor

Once the data module is updated with state exposing capabilities, we could move some of this logic outside the components folder and still access exposed data from the editor module.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 28, 2017

Contributor

Thanks for the review @noisysocks I've fixed the mobile publish button and the i18n issue.
I don't have a better idea for the "switch to draft" button. Open to suggestions and design 👀.

Contributor

youknowriad commented Dec 28, 2017

Thanks for the review @noisysocks I've fixed the mobile publish button and the i18n issue.
I don't have a better idea for the "switch to draft" button. Open to suggestions and design 👀.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Jan 4, 2018

Contributor

Thank you for working on this. Sorry I wasn't here to comment sooner. I think this is 99% there.

Given that the "Saved" affordance more more less disappears when you've published a post, can we simply put the "Switch to Draft" button in the editor bar where the save indicator used to be? Quick and dirty mockup:

screen shot 2018-01-04 at 10 25 20

On mobile we'd keep this button in the sidebar (so you'd have to open the cog). But we'd only want one, i.e. mobile: sidebar. Desktop, only in editor bar.

What do you think?

Contributor

jasmussen commented Jan 4, 2018

Thank you for working on this. Sorry I wasn't here to comment sooner. I think this is 99% there.

Given that the "Saved" affordance more more less disappears when you've published a post, can we simply put the "Switch to Draft" button in the editor bar where the save indicator used to be? Quick and dirty mockup:

screen shot 2018-01-04 at 10 25 20

On mobile we'd keep this button in the sidebar (so you'd have to open the cog). But we'd only want one, i.e. mobile: sidebar. Desktop, only in editor bar.

What do you think?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 4, 2018

Contributor

@jasmussen I personally don't think this action is important enough to put it in the header. But at the same time, I don't have a better option than what we have in this PR.

Contributor

youknowriad commented Jan 4, 2018

@jasmussen I personally don't think this action is important enough to put it in the header. But at the same time, I don't have a better option than what we have in this PR.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Jan 4, 2018

Contributor

I personally don't think this action is important enough to put it in the header. But at the same time, I don't have a better option than what we have in this PR.

I think it actually is — keep in mind that you see this big button ONLY when the pust has been published already. The two key actions you might want to take on such a post is "omfg i published too soon" (perhaps they disabled the publish confirm dialog) and want to just quickly unpublish, or they want to update the post.

This also means if you feel like the button is visually intrusive, you'd only see it once published, so it wouldn't bug you most of the time.

We could visually simplify it by making it into a blue link, like the "Save Draft" action?

Contributor

jasmussen commented Jan 4, 2018

I personally don't think this action is important enough to put it in the header. But at the same time, I don't have a better option than what we have in this PR.

I think it actually is — keep in mind that you see this big button ONLY when the pust has been published already. The two key actions you might want to take on such a post is "omfg i published too soon" (perhaps they disabled the publish confirm dialog) and want to just quickly unpublish, or they want to update the post.

This also means if you feel like the button is visually intrusive, you'd only see it once published, so it wouldn't bug you most of the time.

We could visually simplify it by making it into a blue link, like the "Save Draft" action?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 4, 2018

Contributor

@jasmussen Ok convinced! I'll try 👍

Contributor

youknowriad commented Jan 4, 2018

@jasmussen Ok convinced! I'll try 👍

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 4, 2018

Contributor

Actually, it makes a lot of sense to show it similarily to the "save draft" link.

It can be considered the same link since before publishing it says "save draft" and after it says "switch to draft"

Update the PR with this

Contributor

youknowriad commented Jan 4, 2018

Actually, it makes a lot of sense to show it similarily to the "save draft" link.

It can be considered the same link since before publishing it says "save draft" and after it says "switch to draft"

Update the PR with this

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Jan 4, 2018

Contributor

Here's a screenshot of how it looks now. I love it:

screen shot 2018-01-04 at 12 12 39

I know @mtias has thoughts on this, but I think this is sort of perfect.

I discovered a little bug, though. If you abandon a post mid-publish, the popover returns when you press add new. Steps to reproduce:

  1. Write a new post.
  2. Click "Publish..." but do not publish
  3. Click "Add New"

Observe that your new post starts with the publish dialog already present.

Contributor

jasmussen commented Jan 4, 2018

Here's a screenshot of how it looks now. I love it:

screen shot 2018-01-04 at 12 12 39

I know @mtias has thoughts on this, but I think this is sort of perfect.

I discovered a little bug, though. If you abandon a post mid-publish, the popover returns when you press add new. Steps to reproduce:

  1. Write a new post.
  2. Click "Publish..." but do not publish
  3. Click "Add New"

Observe that your new post starts with the publish dialog already present.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 4, 2018

Contributor

@jasmussen yes, it's tracked here #4248

Contributor

youknowriad commented Jan 4, 2018

@jasmussen yes, it's tracked here #4248

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Jan 4, 2018

Contributor

Perfect, then 👍 👍 from me on the design, though I'd like Matías' thoughts before we merge. Switch to Draft is close to his heart.

Contributor

jasmussen commented Jan 4, 2018

Perfect, then 👍 👍 from me on the design, though I'd like Matías' thoughts before we merge. Switch to Draft is close to his heart.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 5, 2018

Contributor

@mtias waiting for your input here :)

Contributor

youknowriad commented Jan 5, 2018

@mtias waiting for your input here :)

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jan 5, 2018

Contributor

"Switch to draft" at the top looks alright to me, let's try it :)

Contributor

mtias commented Jan 5, 2018

"Switch to draft" at the top looks alright to me, let's try it :)

@youknowriad youknowriad merged commit daab739 into master Jan 5, 2018

3 checks passed

codecov/project 39.16% (-0.02%) compared to ba4e71e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/publish-flow-2 branch Jan 5, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Jan 5, 2018

Contributor

💥

Contributor

jasmussen commented Jan 5, 2018

💥

@@ -33,17 +46,31 @@ function PostPublishPanelToggle( { isSaving, isPublishable, isSaveable, isPublis
disabled={ ! isButtonEnabled }
isBusy={ isSaving && isPublished }
>
<PostPublishButtonLabel />
<Dashicon icon="arrow-down" />
{ __( 'Publish...' ) }

This comment has been minimized.

@aduth

aduth Mar 6, 2018

Member

Poor man's ellipsis: ... (not semantically meaningful)
Rich man's ellipsis: (semantically meaningful)

@aduth

aduth Mar 6, 2018

Member

Poor man's ellipsis: ... (not semantically meaningful)
Rich man's ellipsis: (semantically meaningful)

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