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

Feature: save lock control via actions #10649

Merged
merged 20 commits into from Oct 26, 2018

Conversation

@adamsilverstein
Contributor

adamsilverstein commented Oct 16, 2018

Description

Add a post save locking feature. Plugins can add and remove locks and when any locks are present the publish and update buttons are disabled.

Supersedes #10115
Fixes #7020

How has this been tested?

Load gutenberg and make some edits. In the console, type:
wp.data.dispatch( 'core/editor' ).lockPostSaving( 'mylock' );
Note: the publish or update button becomes disabled.
Remove the post lock with:
wp.data.dispatch( 'core/editor' ).unlockPostSaving( 'mylock' );

  • Test adding multiple locks: the button will remain disabled until all locks are removed.

Types of changes

  • Add actions for lock and unlock post saving.
  • Add an isPostSavingLocked selector
  • Check isPostSavingLocked in post publish button

Checklist:

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

@adamsilverstein adamsilverstein force-pushed the feature/save-lock-control branch from c69d86f to 6db9774 Oct 16, 2018

@adamsilverstein adamsilverstein requested a review from aduth Oct 16, 2018

@aduth

Do we need to consider the post lock for autosaving?

We should have unit tests for at least selectors and reducer.

You need to run and commit changed files from npm run docs:build.

Show resolved Hide resolved packages/editor/src/store/reducer.js
Show resolved Hide resolved packages/editor/src/store/selectors.js Outdated
@adamsilverstein

This comment has been minimized.

Contributor

adamsilverstein commented Oct 18, 2018

Do we need to consider the post lock for autosaving?

I don't think so, these should fire as usual. Autosaves protect the user from lost content.

We should have unit tests for at least selectors and reducer.

I can add these, wanted to make sure approach was acceptable first.

You need to run and commit changed files from npm run docs:build.

Ah, ok will do.

@adamsilverstein

This comment has been minimized.

Contributor

adamsilverstein commented Oct 19, 2018

@aduth I have addressed your feedback and this is ready for review.

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 19, 2018

All the changes look great.

We should have unit tests for at least selectors and reducer.

Let's do that ☝️

We also need to start a new document with UI extensibility for the editor and provide some examples on how to use it in action.

Show resolved Hide resolved packages/editor/src/store/reducer.js Outdated
@adamsilverstein

This comment has been minimized.

Contributor

adamsilverstein commented Oct 19, 2018

@aduth - I addressed your feedback.

@gziolo gziolo added this to In Progress in API freeze via automation Oct 22, 2018

@adamsilverstein

This comment has been minimized.

Contributor

adamsilverstein commented Oct 22, 2018

I'll work on some tests.

@adamsilverstein

This comment has been minimized.

Contributor

adamsilverstein commented Oct 25, 2018

We should have unit tests for at least selectors and reducer.

@gziolo Added tests.

@aduth addressed your feedback, can you please give this another review?

@aduth

aduth approved these changes Oct 25, 2018

My remaining feedback would be good to consider, but are non-blocking improvements.

* @return {boolean} Is locked.
*/
export function isPostSavingLocked( state ) {
return find( state.postSavingLock, ( lock ) => lock ) ? true : false;

This comment has been minimized.

@aduth

aduth Oct 25, 2018

Member

My hunch upon seeing this is: Why can't this just be a condition of Object.keys( state.postSavingLock ).length > 0 ? Why do we hang on to locks in state after the unlock occurs?

Which would require further revision to the reducer to use omit:

case 'LOCK_POST_SAVING':
	return { ...state, [ action.lockName ]: true };

case 'UNLOCK_POST_SAVING':
	return omit( state, action.lockName );

This comment has been minimized.

@adamsilverstein

adamsilverstein Oct 25, 2018

Contributor

yea, that bothered me as well, good suggest - i will refine that

This comment has been minimized.

@adamsilverstein

adamsilverstein Oct 25, 2018

Contributor

Adjusted in 949ed50. Any reason to use Object.keys( state.postSavingLock ).length > 0 instead of state.postSavingLock.length > 0?

This comment has been minimized.

@CGlingener

CGlingener Nov 8, 2018

Contributor

@adamsilverstein there is a very good reason: Object.length is undefined. 🙂 Opened a pull request to fix this little glitch #11636

'test-lock': true,
} );
state = postSavingLock( state, {

This comment has been minimized.

@aduth

aduth Oct 25, 2018

Member

Minor: It's generally a good practice in reducer tests to apply deepFreeze to the state passed to the reducer. It will indirectly protect against unintentional mutations (via thrown errors failing the tests). As written here, technically this test would be satisfied by the following theoretical misbehaving reducer:

export function postSavingLock( state = {}, action ) {
	switch ( action.type ) {
		case 'LOCK_POST_SAVING':
		case 'UNLOCK_POST_SAVING':
			state[ action.lockName ] = action.type === 'LOCK_POST_SAVING';
			break;

	return state;
}

This comment has been minimized.

@adamsilverstein
* @return {PostLockState} Updated state.
*/
export function postSavingLock( state = {}, action ) {
const { lockName } = action;

This comment has been minimized.

@aduth

aduth Oct 25, 2018

Member

Minor: Since a reducer is called on every single action, it's generally best to avoid doing anything until you're certain to be working with a relevant action. On its own, the allocation here is not so concerning, but an unfamiliar future maintainer may perceive this to imply that logic targeting the lock-specific handling is okay to occur prior to switch case, and feel comfortable to make enhancements that at best harm performance and at worst introduce bugs.

tl;dr Best to move into the case handler(s).

This comment has been minimized.

@adamsilverstein

adamsilverstein Oct 25, 2018

Contributor

Adjusted in 50d5a49

@adamsilverstein

This comment has been minimized.

Contributor

adamsilverstein commented Oct 25, 2018

@aduth thanks for the detailed feedback, I think I have addressed your suggestions, one outstanding question then we can merge: #10649 (comment)

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 26, 2018

Thanks @adamsilverstein, great to see we have it finally sorted out 🎉

@gziolo

gziolo approved these changes Oct 26, 2018

@gziolo gziolo merged commit ac81cf7 into master Oct 26, 2018

2 checks passed

codecov/project 48.9% (+0.01%) compared to fb9fa03
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

API freeze automation moved this from In Progress to Done Oct 26, 2018

@gziolo gziolo deleted the feature/save-lock-control branch Oct 26, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 26, 2018

I will open a follow-up with CHANGELOG update, I missed it.

daniloercoli added a commit that referenced this pull request Oct 26, 2018

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…rnmobile/merge-blocks-on-backspace

* 'master' of https://github.com/WordPress/gutenberg:
  Do not add isDirty prop to DOM (#11093)
  Format API (#10209)
  Remove 4.2 deprecated features (#10952)
  Update `@wordpress/hooks` README to include namespace mention (#11061)
  Feature: save lock control via actions (#10649)
  Fix usage of `preg_quote()` (#10998)
  Update plugin version to 4.1.1 (#11078)
  Improve preloading request code (#11007)
  Fix dynamic blocks not rendering in the frontend (#11050)
  Media & Text: Fixing vertical alignment of the image (#11025)
  Date: Mark getSettings as experimental (#10636)
  Improve handling of centered 1-col galleries with small images (#11040)
  Use better help text for ALT text input; fixes #8391. (#11052)

# Conflicts:
#	packages/editor/src/components/rich-text/index.native.js
@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented Nov 9, 2018

@adamsilverstein We've had a few requests for documentation on this so I'm digging in to try to understand how this was implemented. One question I have is why was this implemented just as a save lock, vs separate publish and save locks? Just curious from a conceptual level. I have read the original ticket and the other related closed PRs, but still not sure I understand.

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Feature: save lock control via actions (WordPress#10649)
* Add new actions for lockPostSaving and unlockPostSaving

* isPostSavingLocked selector

* postSavingLock reducer

* check isPostSavingLocked in post publish button

* simplify reducer with object

* adjust isPostSavingLocked for new state shape

* build docs

* postSavingLock - initial state is an empty object

* ensure the isPostSavingLocked selector returns a boolean

* Add tests for isPostSavingLocked

* Add tests for the postSavingLock reducer

* postSavingLock: deepFreeze( state ) passed in tests

* simplify reducers, selectors & adjust tests

* move lockName into case handlers

@CGlingener CGlingener referenced this pull request Nov 22, 2018

Open

Fix disabled publish/update button still clickable #11760

4 of 4 tasks complete
@jaboi

This comment has been minimized.

jaboi commented Nov 24, 2018

@chrisvanpatten Any update on documentation for this? I have a theme that requires a featured image to be horizontal, and the hooks I used to handle the validation and error messaging for this do not work at all with Gutenberg. I think this "save locking" feature is what I'm looking for, but need some kind of documentation to implement. Any feedback would be helpful. Thanks!

@jaboi

This comment has been minimized.

jaboi commented Nov 27, 2018

Does anyone have a working example of this save lock control action for a publish/update conditional?

@adamsilverstein

This comment has been minimized.

Contributor

adamsilverstein commented Dec 14, 2018

We've had a few requests for documentation on this so I'm digging in to try to understand how this was implemented. One question I have is why was this implemented just as a save lock, vs separate publish and save locks? Just curious from a conceptual level. I have read the original ticket and the other related closed PRs, but still not sure I understand.

@chrisvanpatten missed this earlier. likely we chose this approach because this is how the buttons were already architected in regards to other conditionals. plugins should be able to control each button (publish vs update) individually still, happy to help with some example code here.

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