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

Add option to skip PublishSidebar on publishing #9760

Merged
merged 30 commits into from Sep 19, 2018
Merged

Conversation

@nosolosw
Copy link
Member

@nosolosw nosolosw commented Sep 10, 2018

This PR adds a new option to skip the PublishSidebar on publishing.

How it works

The option is surfaced both in the "Tools" section within the general menu and the "Publish sidebar" itself:

peek 2018-09-11 20-24

This is how it looks when the option is enabled:

peek 2018-09-11 20-25

This is how it looks when the option is disabled:

peek 2018-09-11 20-26

Testing

  • Open a new post
  • Add some content
  • Click "Publish..." button (which should show three dots at the end).

The expected result is that the Pre-Publish panel is opened and a checkbox is shown at the bottom of it.

  • Disable the checkbox, and cancel publishing.
  • Click "Publish" button again (which shouldn't have the dots).

The expected result is that the post is published directly, without the pre-publish checks.

These can also be disabled from "Tools" section of the general sidebar. Play a bit with those checks and make sure it works as expected.

TODO

  • Add "Don't show this sidebar again" checkbox to the Publish Sidebar.
  • Better style the dismissable checkbox.
  • Move preference logic to core/editor.
  • Update tests.
@nosolosw nosolosw self-assigned this Sep 10, 2018
@nosolosw nosolosw requested a review from sarahmonster Sep 10, 2018
@nosolosw nosolosw force-pushed the add/skip-publish-panel branch from a8a8bfe to 5869239 Sep 11, 2018
*
* @return {Object} Action object
*/
export function enablePublishSidebar() {

This comment has been minimized.

@nosolosw

nosolosw Sep 11, 2018
Author Member

I originally added the enable/disable logic to the core/edit-post store and moved it later to the core/editor. See 28262d2 The rationale for this change is that the PostPublishPanel (which lives in @wordpress/editor) uses this logic, and the editor can't depend on any component declared in edit-post.

An alternative approach could have been to keep the logic in edit-post and use the PrePublish and PostPublish extensions from edit-post to inject the checkbox component into the Publish sidebar. This is why I've discarded this approach:

  • In my view, the extensions are meant for allowing plugins to inject their own panels into the PublishPanel. Dismissing the panel is a core action and using the extensions to circumvent the editor/edit-post separation would be a convoluted way to add support for it. To me, that'd be a smell that the editor/edit-post separation needs some more thinking.
  • We want the checkbox to be shown in the same position of the panel (the bottom). To achieve this, we'd need to rely on CSS styling, because a plugin can't control other plugins's positions in the panel - which is prone to conflicts and unreliable positioning the checkbox.

This comment has been minimized.

@aduth

aduth Oct 12, 2018
Member

Is there another alternative to consider where the publish panel makes more sense in edit-post and could be moved wholesale there? This doesn't really feel like a component which ought be offered by the editor module.

This comment has been minimized.

@aduth

aduth Oct 12, 2018
Member

I wish we'd taken a more generalized approach akin to edit-post's toggleFeature than to have specific action creators, selectors, reducer handling for each and every user preference.

@nosolosw nosolosw requested review from youknowriad and tofumatt Sep 11, 2018
Copy link
Member

@tofumatt tofumatt left a comment

Since there's still stuff marked todo I didn't check this out and try it yet but I left some comments on the code and approach.

I wonder if it would be better to have the panel appear as a component next to the publish button rather than have the publish button switch between being a button and a panel. It seems a bit odd to place a button and have its component conditionally output one of two very separate things.

I guess if we do it often elsewhere there's precedence, but I don't recall seeing it often in the codebase and I find it a bit less of an explicit/easy-to-follow approach.

Does that make sense?

@@ -38,6 +39,7 @@ const MoreMenu = () => (
filterName="editPost.MoreMenu.tools"
>
<TipsToggle onToggle={ onClose } />
<PublishSidebarToggle onToggle={ onClose } />

This comment has been minimized.

@tofumatt

tofumatt Sep 11, 2018
Member

I know it's unrelated to this PR, but the onClose method being used for onToggle several times and onSelect below makes me think it should be renamed to closeMenu or something. 🤷‍♂️

@@ -0,0 +1,25 @@
import { PostPublishPanelToggle, PostPublishButton } from '@wordpress/editor';

const PublishButton = function( {

This comment has been minimized.

@tofumatt

tofumatt Sep 11, 2018
Member

Any reason for using function() { over () => {? We just tend toward fat-arrow functions in the codebase, so it stood out.

This comment has been minimized.

isSidebarEnabled,
onToggle,
forceIsDirty,
forceIsSaving } ) {

This comment has been minimized.

@tofumatt

tofumatt Sep 11, 2018
Member

Odd that eslint didn't catch it, but I'd expect } ) { on a de-dented newline.

This comment has been minimized.

@@ -0,0 +1,25 @@
import { PostPublishPanelToggle, PostPublishButton } from '@wordpress/editor';

const PublishButton = function( {

This comment has been minimized.

@tofumatt

tofumatt Sep 11, 2018
Member

So this is now a button that turns into a panel? I'd sort of expect we'd have a button that toggled a panel on/off, not really one that changes from panel to button itself...

@@ -96,6 +96,11 @@ class PostPublishPanel extends Component {
{ PostPublishExtension && <PostPublishExtension /> }
</PostPublishPanelPostpublish>
) }
<CheckboxControl
label={ __( "Don't show this sidebar again." ) }

This comment has been minimized.

@tofumatt

tofumatt Sep 11, 2018
Member

This should be an apostrophe (, not a single-quote ').

This comment has been minimized.

@nosolosw

nosolosw Sep 11, 2018
Author Member

I need to update my keyboard setup to include that apostrophe. Good catch!

This comment has been minimized.

@@ -810,6 +810,13 @@ export function preferences( state = PREFERENCES_DEFAULTS, action ) {
...state,
insertUsage: omitBy( state.insertUsage, ( { insert } ) => insert.ref === action.id ),
};

case 'ENABLE_PUBLISH_SIDEBAR':
case 'DISABLE_PUBLISH_SIDEBAR':

This comment has been minimized.

@tofumatt

tofumatt Sep 11, 2018
Member

I get that this is technically less code but I think it's nicer to have each action type return its own object. It's easier to grok what's happening that way–this code made sense to me but only after I looked at it for probably ten seconds; I was wondering why 'DISABLE_PUBLISH_SIDEBAR' needed to check if the action was 'ENABLE_PUBLISH_SIDEBAR'. I think it'd be nicer to just do:

case 'ENABLE_PUBLISH_SIDEBAR':
	return {
		...state,
		isPublishSidebarEnabled: true,
	};
case 'DISABLE_PUBLISH_SIDEBAR':
	return {
		…state,
		isPublishSidebarEnabled: false,
	};

This comment has been minimized.

@nosolosw

nosolosw Sep 11, 2018
Author Member

Thanks for keeping us obvious.

This comment has been minimized.

Copy link
Member

@sarahmonster sarahmonster left a comment

So! When I tested this out the first time, it defaulted to not-showing the pre-publish panel. I think it would be better if we made the pre-publish panel opt-out, rather than opt-in. That is, you can hide it if you want to, but by default, it's shown.

Next! Let's avoid calling it a "sidebar", since a) it's not exactly behaving as a sidebar once all our pre-publish panel changes land in #9398 and b) it's not a sidebar at all on mobile. I keep waffling on what to call it and how to phrase this, so I think it'd be good to put out a batsignal for some editorial help!

Some ideas:

  • pre-publish panel
  • publish checkup
  • publish confirmation

I've realised we should also probably match the checkbox behaviour—so if the checkbox is checked in the tools menu, it should also be checked in the pre-publish panel. That will involve more wording changes, and it might make sense to make it a positive ("show the panel" rather than a negative ("don't show the panel), but it could work either way, so long as we ensure their states match.

In the tools menu:
"Show publish confirmation" (positive)
or "Publish posts immediately" (negative)

And matching statements for the panel checkbox:
"Show this confirmation every time I publish a post" (positive)
or "Don't show this next time I publish a post" (negative)

These would ideally be a bit shorter if possible too.

Third, I'll make a wee CSS change here that'll fix the margins issue for this PR, even though it'll just be a band-aid until we get all the other changes introduced as well.

@nosolosw
Copy link
Member Author

@nosolosw nosolosw commented Sep 11, 2018

Third, I'll make a wee CSS change here that'll fix the margins issue for this PR, even though it'll just be a band-aid until we get all the other changes introduced as well.

I was already working on that! Changes already pushed.

@nosolosw
Copy link
Member Author

@nosolosw nosolosw commented Sep 11, 2018

So! When I tested this out the first time, it defaulted to not-showing the pre-publish panel. I think it would be better if we made the pre-publish panel opt-out, rather than opt-in. That is, you can hide it if you want to, but by default, it's shown.

mmm, I've tried by clearing my browser's cache and deleting any local changes I might have and it works as an opt-in. That was the intention. Perhaps an earlier bug? Would you mind clearing your browser's cache and try again?

Updated the wording and changed the state of checkbox and tools menu item to be equal.

@nosolosw
Copy link
Member Author

@nosolosw nosolosw commented Sep 11, 2018

@tofumatt I'll defer to @sarahmonster and @karmatosed about the interaction, as I know they've discussed it and explored options in #7602 and #9398

@tofumatt tofumatt self-requested a review Sep 11, 2018
@michelleweber
Copy link

@michelleweber michelleweber commented Sep 11, 2018

I'd call it "pre-publish check" and I'm thinking the clearest thing is to use to turn it on is the same language as the confirmation: "Double-check settings before publication" -- If that's too long, maybe just "Enable pre-publish checks" -- and "Prompt me to double-check settings every time I publish something."

@sarahmonster
Copy link
Member

@sarahmonster sarahmonster commented Sep 11, 2018

"Enable pre-publish checks" is probably best for the toolbar given length, and it's very clear. 💯

What about making the checkbox in the panel itself mirror this wording more directly—something along the lines of "Show this pre-publish check every time I publish" or "Always show pre-publish checks."?

@nosolosw
Copy link
Member Author

@nosolosw nosolosw commented Sep 12, 2018

Updated copy in d7d1b77

screenshot from 2018-09-12 09-30-58

screenshot from 2018-09-12 09-31-32

@nosolosw nosolosw added this to the 3.9 milestone Sep 12, 2018
@sarahmonster
Copy link
Member

@sarahmonster sarahmonster commented Sep 12, 2018

Perhaps an earlier bug? Would you mind clearing your browser's cache and try again?

Seems to be working as expected now, or at least I can't replicate anymore, so 👍.

I wonder if it would be better to have the panel appear as a component next to the publish button rather than have the publish button switch between being a button and a panel. It seems a bit odd to place a button and have its component conditionally output one of two very separate things.

I've tried to parse this several times and can't make heads or tails of it, but if you can clarify @tofumatt I'm happy to give feedback on the interaction pattern!

@tofumatt
Copy link
Member

@tofumatt tofumatt commented Sep 12, 2018

Sorry, my comment was just about coding; the interaction would be the same, but I thought it was strange that the component conditionally output two very separate components based on state.

@nosolosw
Copy link
Member Author

@nosolosw nosolosw commented Sep 12, 2018

Sorry, my comment was just about coding; the interaction would be the same, but I thought it was strange that the component conditionally output two very separate components based on state.

Do you refer to https://github.com/WordPress/gutenberg/pull/9760/files#diff-a6f2fb9ba2b6fbe3ad649f4c019b76deR53 right? Conditionally show a button or a toggle depending on whether the pre-publish checks are enabled or not. I'm not sure I understand your concerns. Could we perhaps move to code and see if there is a different implementation? Let's comment there.

@tofumatt
Copy link
Member

@tofumatt tofumatt commented Sep 12, 2018

@nosolosw Looks like you changed the code and alleviated my concerns 😄

Also: is the code ready for another review? I can look if so 😄

@nosolosw
Copy link
Member Author

@nosolosw nosolosw commented Sep 12, 2018

Also: is the code ready for another review? I can look if so

Please! Also cc @sarahmonster in case she has any design concerns I should address. It'd be lovely to merge this today or tomorrow so it can make it to the 3.9 release.

Copy link
Member

@tofumatt tofumatt left a comment

Looks good, but when I checked it out and tested it the pre-publish panel wasn't on by default, which I'd think it should be.

I saw that eventually going to another page or something fixed it for @sarahmonster… but as I already think this could use some testing I'd like to see an E2E test that makes sure it's enabled by default on a new site and that it can be disabled before merging.

Code and experience works great though, nice job 👍

@@ -97,6 +97,13 @@ class PostPublishPanel extends Component {
</PostPublishPanelPostpublish>
) }
</div>
<div className="editor-post-publish-panel__footer">
<CheckboxControl
label={ __( 'Always show pre-publish checks.' ) }

This comment has been minimized.

@tofumatt

tofumatt Sep 12, 2018
Member

Putting the word "this" inside this text might help contextualise this more.

Always show these pre-publish checks.

might help users understand it more. If it doesn't look too cramped I say go with that, but if it's already a tight space feel free to leave it as-is.

nosolosw and others added 12 commits Sep 11, 2018
These data has been moved to edit-post store as per
#4661
In theory, isPublishSidebarEnabled shouldn't be ever undefined
in the preferences object, so this shouldn't be necessary.

In practice, we use a localStorage mechanism to persist
parts of the state in between sessions (like whether the user
has already seen the tips or not). For the core/editor store
we persist the preferences key, which contains an object.
This is the problem: the localStorage mechanism doesn't check
whether the cached preferences object and the state preferences object
have the same shape, it just overwrites the current state,
so new added keys are deleted.

While this bug is not fixed and deployed,
we want this preventive measure in place.
@nosolosw nosolosw force-pushed the add/skip-publish-panel branch from 3548cd0 to 87b372d Sep 17, 2018
@nosolosw nosolosw requested a review from tofumatt Sep 17, 2018
@nosolosw
Copy link
Member Author

@nosolosw nosolosw commented Sep 17, 2018

I think it would be straightforward to write and useful to test.

After you learn puppeteer, it may be argued they are easy to write. I'm still undecided whether the puppeteer API is simple, though. Once written, no doubt they are very useful. Thanks for prompting me to go further, I've learned something new!

I've also taken the time to refactor the e2e code to make it concise addressing these concerns.

@nosolosw
Copy link
Member Author

@nosolosw nosolosw commented Sep 18, 2018

Tested scheduling & submit for review states, on mobile, translations, etc.

@tofumatt I can land this as soon as I have a 👍 to enable merging again.

Copy link
Member

@tofumatt tofumatt left a comment

I think we need to reset the setting for pre-publish panel being disabled and I left a comment about a selector, but otherwise this looks good.

Once the tests reset the setting so they aren't causing side-effects outside their test runs this is good to merge, so approving conditional on that change 😄

describe( `a ${ postType } with pre-publish checks disabled`, () => {
beforeEach( async () => {
await newPost( postType );
await disablePrePublishChecks();

This comment has been minimized.

@tofumatt

tofumatt Sep 18, 2018
Member

Because this setting persists between page loads we should be careful with this… arguably we should have some kind of consistent/known state and after each test where we disable the pre-publish check we should re-enable them with the afterEach test helper.

Right now this test will disable pre-publish checks elsewhere which–as it's not the default state–could be confusing to future test writers.

This comment has been minimized.

@nosolosw

nosolosw Sep 19, 2018
Author Member

Done, good catch!

export async function disablePrePublishChecks( ) {
await page.click( '.edit-post-more-menu' );
await page.waitForSelector( '.components-popover__content' );
await page.click( '.components-popover__content > .components-menu-group:nth-child(3) button:nth-child(3)' );

This comment has been minimized.

@tofumatt

tofumatt Sep 18, 2018
Member

This is a bit of a fragile selector, is there anything better/better-named we could use to select the "Don't show this again" checkbox? It looks like . editor-post-publish-panel__footer input[type=checkbox] would work assuming CheckboxControl outputs an actual checkbox.

This comment has been minimized.

@nosolosw

nosolosw Sep 19, 2018
Author Member

Solved at d7c7151

I was reluctant to introduce a specific selector just for testing but, on a second thought, it's totally worth it in this case. Note also that the code uses the general menu, not the actual checkbox within the publish sidebar. We can't guarantee the publish sidebar is enabled unless we use the general menu.

@nosolosw nosolosw merged commit 27cc2b6 into master Sep 19, 2018
2 checks passed
2 checks passed
codecov/project 48.74% (-0.02%) compared to 2b42db5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nosolosw nosolosw deleted the add/skip-publish-panel branch Sep 19, 2018
@mintplugins
Copy link

@mintplugins mintplugins commented Sep 21, 2018

Just to throw another opinion out there, has the problem of having multiple User Experiences been raised as a concern here? If some people experience it one way, and others experience it another way (based on a setting they'll possibly/likely forget about), it creates a disparate UX, making it tougher to write tutorials, make tutorial videos, and even to develop plugins which tap into this area.

Personally I don't like this area, but I think I like having the option to turn it off even less, as it created a forever-more fragmented user experience. My apologies for being a downer!

} );
} );

it( 'should disable the publish sidebar', () => {
const original = deepFreeze( preferences( undefined, { } ) );

This comment has been minimized.

@aduth

aduth Oct 12, 2018
Member

No filler spaces in empty constructs (e.g., {}, [], fn()).

https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#spacing

Need to find if possible to enforce by ESLint.

This comment has been minimized.

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

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.