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

WIP: Try new publishing flow #9398

Closed
wants to merge 7 commits into from
Closed

Conversation

sarahmonster
Copy link
Member

@sarahmonster sarahmonster commented Aug 28, 2018

This is my attempt to start implementing publishing flow changes in #7602. I've done some of the cosmetic changes, but some of the more heavy-lifting will need a friendly developer, especially since some of the code in here I'm finding a bit hard to follow.

I've tried some bigger changes here, with respect to adding a lot more spacing between elements and adding some typographic hierarchy. It may be a bit much here, since it's inconsistent with styling used elsewhere, but we can always dial it back if it feels too disjointed.

Before:

2018-08-28 12 23 25

After:

2018-08-28 11 16 43

What's missing?

  • scrolling should only scroll the panel, not the window itself
  • dimming the background via a scrim would be good, and I think this would also solve the interaction issue where the page gets scrolled rather than the panel, by making the page itself non-interactive
  • might make sense to refactor so there's pre-publish-panel and post-publish-panel, since they are now (or will be) more separate entities, and there's a lot of functionality changes happening here—I started doing this but got a bit lost in the weeds, so I'd appreciate some help there
  • panel title doesn't seem to be behaving properly, I think because I've moved it up a level and the logic is different
  • there's an awkward lag for suggestion panel to come up (not introduced via this PR but could be nice to get fixed if it's relatively trivial)
  • the checkbox for "don't show this again" should probably not show this again (via an option)
    - [ ] clicking "private" toggle immediately publishes the post (Setting post visibility to "private" forces immediate publishing of post #9396)
  • changed the password input field to be more consistent with the other inputs and it broke things (will file separately)

Next phase

I've only dealt with the pre-publish panel right now, mostly due to technical limitations. Here's what I'd like for the post-publish phase:

There should be one state for "publishing" and one for "published" (both of which currently exist), and then, once the post has published, we should redirect to show the post, with a modal overlay giving some next-step advise as per the mockup.

I can adjust the styling of the "publishing..." and the "published!" states, but it would help to be able to get them to display temporarily, which I'm having troubles making happen. (Setting submitted: true on PostPublishPanel brings up the published state, but I'm having troubles getting the publishing state to stay on the screen for any length of time, making it super difficult to style or adjust since it disappears quite quickly!)

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Cool. I like the increase in size; the old one felt cramped/small and it didn't get my attention because of that.

Just a few passing thoughts; I know this isn't complete.

color: $blue-dark-900;
font-weight: 400;
padding-left: 4px;
//text-decoration: underline;
Copy link
Member

Choose a reason for hiding this comment

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

🔥


<div className="editor-post-publish-panel__disable-check">
<CheckboxControl
label={ __( 'Don’t show this again' ) }
Copy link
Member

Choose a reason for hiding this comment

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

"Don’t show this panel again" might be better; I'm often wary about an unclear "this".

@@ -61,22 +61,21 @@ class PostPublishPanel extends Component {
this.setState( { loading: true } );
}

onUpdateDisableToggle() {
// There should be a setting to allow users to skip the pre-publish step forever.
Copy link
Member

Choose a reason for hiding this comment

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

That'll need to be a user setting, I'd think. I don't know how we set those in Gutenberg, but this should dispatch an action that makes an API request to toggle said setting.

@oandregal
Copy link
Member

scrolling should only scroll the panel, not the window itself

It works for me in Chrome and Firefox:

peek 2018-09-06 17-33

@oandregal
Copy link
Member

I've got a couple of questions about the "Don't show this again" checkbox that affects implementation

  1. If an user checks it, how can that be unchecked if the pre-publish panel is not ever shown?

One potential solution would be to leverage the fact that the pre-publish panel only shows settings that can be modified elsewhere. What if we do the same for this one? I can think of two places for this:

  • In the "Status & Visibility" metabox within the Document sidebar.
  • In the "Tools" section of the general Gutenberg menu (ellipsis at the top right).

I think it conceptually fits within the "Tools" section. Thoughts?

  1. Shall we change the publish button text from "Publish..." to "Publish" depending on whether the pre-publish panel is going to be shown or not?

That'd be more in line with the intent of adding dots to the Publish text (convey that it's more than just publish...), but wanted to check first.

@sarahmonster
Copy link
Member Author

For the user setting, I was thinking it'd make most sense to have it under the "Tools" section, since that's where other user settings are. It's pretty similar conceptually to the "Show tips" option that appears here as well. So, basically, your instincts here are bang-on and exactly what I was thinking (but didn't put in the mockup). 💯

Shall we change the publish button text from "Publish..." to "Publish" depending on whether the pre-publish panel is going to be shown or not?

Yes please!

@oandregal
Copy link
Member

Sarah, #9760 adds the ability to skip the publish sidebar on publishing (work in progress). It may be worth removing the changes to make the sidebar dismissable in this PR.

@sarahmonster
Copy link
Member Author

Yep, makes sense! Once that PR is finished I'll rebase this one on master and clean things up a bit, and then we can loop back to this.

@sarahmonster
Copy link
Member Author

I've now rebased this to take into account other changes, and it's working a bit better. Here's what's left that I need help with in order for this to be merge-ready!

  • add a background scrim to nix interactivity & dim the background
  • redirect to post after publishing and show a dismissable post-publish modal
  • help with testing out different states for publishing button, so I can customise them in CSS (ie, can I set a state temporarily for testing)

@karmatosed
Copy link
Member

Just one point on this, can we right align the publish button please?

@gziolo
Copy link
Member

gziolo commented Jan 31, 2019

It looks like this one was abandoned, what is the plan moving forward with those changes proposed? Do you plan to refresh this PR? I'm labeling it as Stale to make PR triaging easier but feel free to remove it as soon as you refresh PR.

@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jan 31, 2019
@gziolo gziolo requested a review from mapk January 31, 2019 13:50
@gziolo gziolo added the [Type] Enhancement A suggestion for improvement. label Jan 31, 2019
@sarahmonster
Copy link
Member Author

I'm going to go ahead and close this. To implement the changes suggested in #7602, I'd need development help—this PR was as far as I could get with it.

If it's a priority to implement these changes, I'm happy to revisit in a new PR, but I would need help addressing the technical issues outlined above.

@youknowriad youknowriad deleted the try/new-publishing-flow branch May 27, 2020 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants