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
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
b7cc241
Add PrePublish panel option to the general menu tools section
oandregal Sep 7, 2018
e3f0c24
Add data API
oandregal Sep 7, 2018
85edadc
Rename PublishPanel to PrePublishPanel
oandregal Sep 7, 2018
0488595
Implement core/edit-post actions and selector
oandregal Sep 10, 2018
14ea423
Rename pre-publish-panel-toggle to publish-sidebar-toggle
oandregal Sep 10, 2018
f4c237e
Connect the dots: publish toggles publish sidebar or publish directly
oandregal Sep 10, 2018
1a00c13
Update test for isPublishSidebarEnable preference
oandregal Sep 11, 2018
c659529
Expose dismissing option as a checkbox in the sidebar
oandregal Sep 11, 2018
e888220
Move publish sidebar logic to core/editor store
oandregal Sep 11, 2018
b3e9fa8
docs: tweak comment on isPublishSidebarEnabled
tofumatt Sep 11, 2018
f78a332
Use arrow function instead of traditional function
oandregal Sep 11, 2018
3e2877f
Fix linting issue that went undetected by eslint
oandregal Sep 11, 2018
dbd6090
Use english apostrophe, not single commas
oandregal Sep 11, 2018
ea5dc6a
Make enable/disable actions more obvious
oandregal Sep 11, 2018
afae18d
Inline PostPublish button / toggle logic
oandregal Sep 11, 2018
2f45e20
Add padding to align and give it space
oandregal Sep 11, 2018
7bfc2a5
Align to bottom
oandregal Sep 11, 2018
c65ea7c
Update wording
oandregal Sep 11, 2018
18036e2
Update copy
oandregal Sep 12, 2018
c37b6a8
Tweak microcopy for pre-publish checks.
sarahmonster Sep 12, 2018
d1c69c5
Remove outdated docs
oandregal Sep 13, 2018
1be0b54
Preventive measure
oandregal Sep 14, 2018
79ced01
Fix and add tests
oandregal Sep 14, 2018
78ebeec
Add e2e test
oandregal Sep 17, 2018
87b372d
Refactor code to avoid duplication
oandregal Sep 17, 2018
acffe70
chore: Tweak styles
tofumatt Sep 18, 2018
37900cc
Disable pre-publish checks after test
oandregal Sep 19, 2018
6637b7e
Add enable/disable functions
oandregal Sep 19, 2018
d7c7151
Use specific selector to target pre-publish checks menu item
oandregal Sep 19, 2018
b50ca1d
Do not make assumptions about previous states
oandregal Sep 19, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions edit-post/components/header/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
PostPreviewButton,
PostSavedState,
PostPublishPanelToggle,
PostPublishButton,
} from '@wordpress/editor';
import { withDispatch, withSelect } from '@wordpress/data';
import { compose } from '@wordpress/compose';
Expand All @@ -26,6 +27,7 @@ function Header( {
openGeneralSidebar,
closeGeneralSidebar,
isPublishSidebarOpened,
isPublishSidebarEnabled,
togglePublishSidebar,
hasActiveMetaboxes,
isSaving,
Expand All @@ -48,12 +50,19 @@ function Header( {
forceIsSaving={ isSaving }
/>
<PostPreviewButton />
<PostPublishPanelToggle
isOpen={ isPublishSidebarOpened }
onToggle={ togglePublishSidebar }
forceIsDirty={ hasActiveMetaboxes }
forceIsSaving={ isSaving }
/>
{ isPublishSidebarEnabled ? (
<PostPublishPanelToggle
isOpen={ isPublishSidebarOpened }
onToggle={ togglePublishSidebar }
forceIsDirty={ hasActiveMetaboxes }
forceIsSaving={ isSaving }
/>
) : (
<PostPublishButton
forceIsDirty={ hasActiveMetaboxes }
forceIsSaving={ isSaving }
/>
) }
<div>
<IconButton
icon="admin-generic"
Expand All @@ -79,6 +88,7 @@ export default compose(
withSelect( ( select ) => ( {
isEditorSidebarOpened: select( 'core/edit-post' ).isEditorSidebarOpened(),
isPublishSidebarOpened: select( 'core/edit-post' ).isPublishSidebarOpened(),
isPublishSidebarEnabled: select( 'core/editor' ).isPublishSidebarEnabled(),
hasActiveMetaboxes: select( 'core/edit-post' ).hasMetaBoxes(),
isSaving: select( 'core/edit-post' ).isSavingMetaBoxes(),
hasBlockSelection: !! select( 'core/editor' ).getBlockSelectionStart(),
Expand Down
2 changes: 2 additions & 0 deletions edit-post/components/header/more-menu/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import PluginMoreMenuGroup from '../plugins-more-menu-group';
import TipsToggle from '../tips-toggle';
import KeyboardShortcutsHelpMenuItem from '../keyboard-shortcuts-help-menu-item';
import WritingMenu from '../writing-menu';
import PublishSidebarToggle from '../publish-sidebar-toggle';

const MoreMenu = () => (
<Dropdown
Expand Down Expand Up @@ -44,6 +45,7 @@ const MoreMenu = () => (
{ __( 'Manage All Reusable Blocks' ) }
</MenuItem>
<TipsToggle onToggle={ onClose } />
<PublishSidebarToggle onToggle={ onClose } />
Copy link
Member

Choose a reason for hiding this comment

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

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. 🤷‍♂️

<KeyboardShortcutsHelpMenuItem onSelect={ onClose } />
</MenuGroup>
</Fragment>
Expand Down
37 changes: 37 additions & 0 deletions edit-post/components/header/publish-sidebar-toggle/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/**
* WordPress Dependencies
*/
import { __ } from '@wordpress/i18n';
import { MenuItem } from '@wordpress/components';
import { compose } from '@wordpress/compose';
import { withSelect, withDispatch } from '@wordpress/data';

const PublishSidebarToggle = function( { onToggle, isEnabled } ) {
return (
<MenuItem
icon={ isEnabled && 'yes' }
tofumatt marked this conversation as resolved.
Show resolved Hide resolved
isSelected={ isEnabled }
role="menuitemcheckbox"
onClick={ onToggle }
>
{ __( 'Enable Pre-publish Checks' ) }
</MenuItem>
);
};

export default compose( [
withSelect( ( select ) => ( {
isEnabled: select( 'core/editor' ).isPublishSidebarEnabled(),
} ) ),
withDispatch( ( dispatch, ownProps ) => ( {
onToggle() {
const { disablePublishSidebar, enablePublishSidebar } = dispatch( 'core/editor' );
if ( ownProps.isEnabled ) {
disablePublishSidebar();
} else {
enablePublishSidebar();
}
ownProps.onToggle();
},
} ) ),
] )( PublishSidebarToggle );
27 changes: 24 additions & 3 deletions packages/editor/src/components/post-publish-panel/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import { get } from 'lodash';
*/
import { __ } from '@wordpress/i18n';
import { Component } from '@wordpress/element';
import { IconButton, Spinner } from '@wordpress/components';
import { withSelect } from '@wordpress/data';
import { IconButton, Spinner, CheckboxControl } from '@wordpress/components';
import { withSelect, withDispatch } from '@wordpress/data';
import { compose } from '@wordpress/compose';

/**
Expand Down Expand Up @@ -62,7 +62,7 @@ class PostPublishPanel extends Component {
}

render() {
const { isScheduled, onClose, forceIsDirty, forceIsSaving, PrePublishExtension, PostPublishExtension, ...additionalProps } = this.props;
const { isScheduled, isPublishSidebarEnabled, onClose, onTogglePublishSidebar, forceIsDirty, forceIsSaving, PrePublishExtension, PostPublishExtension, ...additionalProps } = this.props;
const { loading, submitted } = this.state;
return (
<div className="editor-post-publish-panel" { ...additionalProps }>
Expand Down Expand Up @@ -97,6 +97,13 @@ class PostPublishPanel extends Component {
</PostPublishPanelPostpublish>
) }
</div>
<div className="editor-post-publish-panel__footer">
<CheckboxControl
label={ __( 'Always show pre-publish checks.' ) }
Copy link
Member

Choose a reason for hiding this comment

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

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.

checked={ isPublishSidebarEnabled }
onChange={ onTogglePublishSidebar }
/>
</div>
</div>
);
}
Expand All @@ -112,13 +119,27 @@ export default compose( [
isSavingPost,
isEditedPostDirty,
} = select( 'core/editor' );
const { isPublishSidebarEnabled } = select( 'core/editor' );
return {
postType: getCurrentPostType(),
hasPublishAction: get( getCurrentPost(), [ '_links', 'wp:action-publish' ], false ),
isPublished: isCurrentPostPublished(),
isScheduled: isCurrentPostScheduled(),
isSaving: isSavingPost(),
isDirty: isEditedPostDirty(),
isPublishSidebarEnabled: isPublishSidebarEnabled(),
};
} ),
withDispatch( ( dispatch, { isPublishSidebarEnabled } ) => {
const { disablePublishSidebar, enablePublishSidebar } = dispatch( 'core/editor' );
return {
onTogglePublishSidebar: ( ) => {
if ( isPublishSidebarEnabled ) {
disablePublishSidebar();
} else {
enablePublishSidebar();
}
},
};
} ),
] )( PostPublishPanel );
6 changes: 6 additions & 0 deletions packages/editor/src/components/post-publish-panel/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@
flex-grow: 1;
}

.editor-post-publish-panel__footer {
padding: 16px;
position: absolute;
bottom: 0;
}

.components-button.editor-post-publish-panel__toggle.is-primary {
display: inline-flex;
align-items: center;
Expand Down
22 changes: 22 additions & 0 deletions packages/editor/src/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -763,3 +763,25 @@ export function unregisterToken( name ) {
name,
};
}

/**
* Returns an action object used in signalling that the user has enabled the publish sidebar.
*
* @return {Object} Action object
*/
export function enablePublishSidebar() {
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

return {
type: 'ENABLE_PUBLISH_SIDEBAR',
};
}

/**
* Returns an action object used in signalling that the user has disabled the publish sidebar.
*
* @return {Object} Action object
*/
export function disablePublishSidebar() {
return {
type: 'DISABLE_PUBLISH_SIDEBAR',
};
}
1 change: 1 addition & 0 deletions packages/editor/src/store/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { __ } from '@wordpress/i18n';

export const PREFERENCES_DEFAULTS = {
insertUsage: {},
isPublishSidebarEnabled: true,
};

/**
Expand Down
15 changes: 12 additions & 3 deletions packages/editor/src/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -773,9 +773,6 @@ export function settings( state = EDITOR_SETTINGS_DEFAULTS, action ) {
* Reducer returning the user preferences.
*
* @param {Object} state Current state.
* @param {string} state.mode Current editor mode, either "visual" or "text".
* @param {boolean} state.isSidebarOpened Whether the sidebar is opened or closed.
* @param {Object} state.panels The state of the different sidebar panels.
* @param {Object} action Dispatched action.
*
* @return {string} Updated state.
Expand Down Expand Up @@ -810,6 +807,18 @@ export function preferences( state = PREFERENCES_DEFAULTS, action ) {
...state,
insertUsage: omitBy( state.insertUsage, ( { insert } ) => insert.ref === action.id ),
};

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

case 'DISABLE_PUBLISH_SIDEBAR':
Copy link
Member

Choose a reason for hiding this comment

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

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,
	};

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for keeping us obvious.

Copy link
Member Author

Choose a reason for hiding this comment

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

return {
...state,
isPublishSidebarEnabled: false,
};
}

return state;
Expand Down
20 changes: 20 additions & 0 deletions packages/editor/src/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ import { serialize, getBlockType, getBlockTypes, hasBlockSupport, hasChildBlocks
import { moment } from '@wordpress/date';
import { removep } from '@wordpress/autop';

/**
* Dependencies
*/
import { PREFERENCES_DEFAULTS } from './defaults';

/***
* Module constants
*/
Expand Down Expand Up @@ -1879,3 +1884,18 @@ export function getTokenSettings( state, name ) {
export function canUserUseUnfilteredHTML( state ) {
return has( getCurrentPost( state ), [ '_links', 'wp:action-unfiltered_html' ] );
}

/**
* Returns whether the pre-publish panel should be shown
* or skipped when the user clicks the "publish" button.
*
* @param {Object} state Global application state.
*
* @return {boolean} Whether the pre-publish panel should be shown or not.
*/
export function isPublishSidebarEnabled( state ) {
if ( state.preferences.hasOwnProperty( 'isPublishSidebarEnabled' ) ) {
return state.preferences.isPublishSidebarEnabled;
}
return PREFERENCES_DEFAULTS.isPublishSidebarEnabled;
}
19 changes: 19 additions & 0 deletions packages/editor/src/store/test/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1580,9 +1580,28 @@ describe( 'state', () => {

expect( state ).toEqual( {
insertUsage: {},
isPublishSidebarEnabled: true,
} );
} );

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

const state = preferences( original, {
type: 'DISABLE_PUBLISH_SIDEBAR',
} );

expect( state.isPublishSidebarEnabled ).toBe( false );
} );

it( 'should enable the publish sidebar', () => {
const original = deepFreeze( preferences( { isPublishSidebarEnabled: false }, { } ) );
const state = preferences( original, {
type: 'ENABLE_PUBLISH_SIDEBAR',
} );

expect( state.isPublishSidebarEnabled ).toBe( true );
} );

it( 'should record recently used blocks', () => {
const state = preferences( deepFreeze( { insertUsage: {} } ), {
type: 'INSERT_BLOCKS',
Expand Down
29 changes: 29 additions & 0 deletions packages/editor/src/store/test/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { moment } from '@wordpress/date';
* Internal dependencies
*/
import * as selectors from '../selectors';
import { PREFERENCES_DEFAULTS } from '../defaults';

const {
canUserUseUnfilteredHTML,
Expand Down Expand Up @@ -82,6 +83,7 @@ const {
getReusableBlocks,
getStateBeforeOptimisticTransaction,
isPublishingPost,
isPublishSidebarEnabled,
canInsertBlockType,
getInserterItems,
isValidTemplate,
Expand Down Expand Up @@ -3387,6 +3389,33 @@ describe( 'selectors', () => {
} );
} );

describe( 'isPublishSidebarEnabled', () => {
it( 'should return the value on state if it is thruthy', () => {
const state = {
preferences: {
isPublishSidebarEnabled: true,
},
};
expect( isPublishSidebarEnabled( state ) ).toBe( state.preferences.isPublishSidebarEnabled );
} );

it( 'should return the value on state if it is falsy', () => {
const state = {
preferences: {
isPublishSidebarEnabled: false,
},
};
expect( isPublishSidebarEnabled( state ) ).toBe( state.preferences.isPublishSidebarEnabled );
} );

it( 'should return the default value if there is no isPublishSidebarEnabled key on state', () => {
const state = {
preferences: { },
};
expect( isPublishSidebarEnabled( state ) ).toBe( PREFERENCES_DEFAULTS.isPublishSidebarEnabled );
} );
} );

describe( 'isFetchingReusableBlock', () => {
it( 'should return false when the block is not being fetched', () => {
const state = {
Expand Down
Loading