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.

Sign up for GitHub

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

Jump to bottom

Use experiment locking/unlocking system for block interface selector and actions #47375

Merged
merged 6 commits into from
Jan 30, 2023
Merged

Use experiment locking/unlocking system for block interface selector and actions #47375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3be14b4
Update block editor store to make the block interface API properly ex…
talldan Jan 24, 2023
83e317f
Unlock experimental block interface API before usage
talldan Jan 24, 2023
000469c
Fix tests
talldan Jan 27, 2023
54513ea
Remove __experimental prefix from locked APIs
talldan Jan 27, 2023
a69e0d4
Update documentation
talldan Jan 30, 2023
f9f5188
Fix incorrect `privateSettings` key
talldan Jan 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/block-editor/src/components/block-tools/selected-block-popover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,18 @@ import { store as blockEditorStore } from '../../store';
import BlockPopover from '../block-popover';
import useBlockToolbarPopoverProps from './use-block-toolbar-popover-props';
import Inserter from '../inserter';
import { unlock } from '../../experiments';

function selector( select ) {
const {
__unstableGetEditorMode,
isMultiSelecting,
hasMultiSelection,
isTyping,
__experimentalIsBlockInterfaceHidden: isBlockInterfaceHidden,
isBlockInterfaceHidden,
getSettings,
getLastMultiSelectedBlockClientId,
} = select( blockEditorStore );
} = unlock( select( blockEditorStore ) );
adamziel marked this conversation as resolved.
Show resolved Hide resolved

return {
editorMode: __unstableGetEditorMode(),
Expand Down
8 changes: 4 additions & 4 deletions packages/block-editor/src/hooks/dimensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ import {
} from './child-layout';
import useSetting from '../components/use-setting';
import { store as blockEditorStore } from '../store';
import { unlock } from '../experiments';

export const DIMENSIONS_SUPPORT_KEY = 'dimensions';
export const SPACING_SUPPORT_KEY = 'spacing';
Expand All @@ -67,10 +68,9 @@ export const AXIAL_SIDES = [ 'vertical', 'horizontal' ];

function useVisualizerMouseOver() {
const [ isMouseOver, setIsMouseOver ] = useState( false );
const {
__experimentalHideBlockInterface: hideBlockInterface,
__experimentalShowBlockInterface: showBlockInterface,
} = useDispatch( blockEditorStore );
const { hideBlockInterface, showBlockInterface } = unlock(
useDispatch( blockEditorStore )
);
const onMouseOver = ( e ) => {
e.stopPropagation();
hideBlockInterface();
Expand Down
63 changes: 1 addition & 62 deletions packages/block-editor/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import { speak } from '@wordpress/a11y';
import { __, _n, sprintf } from '@wordpress/i18n';
import { create, insert, remove, toHTMLString } from '@wordpress/rich-text';
import deprecated from '@wordpress/deprecated';
import { Platform } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -26,6 +25,7 @@ import {
retrieveSelectedAttribute,
START_OF_SELECTED_AREA,
} from '../utils/selection';
import { __experimentalUpdateSettings } from './private-actions';

/** @typedef {import('../components/use-on-block-drop/types').WPDropOperation} WPDropOperation */

Expand Down Expand Up @@ -1264,28 +1264,6 @@ export function toggleBlockMode( clientId ) {
};
}

/**
* Returns an action object used in signalling that the block interface, eg. toolbar, outline, etc. should be hidden.
*
* @return {Object} Action object.
*/
export function __experimentalHideBlockInterface() {
return {
type: 'HIDE_BLOCK_INTERFACE',
};
}

/**
* Returns an action object used in signalling that the block interface, eg. toolbar, outline, etc. should be shown.
*
* @return {Object} Action object.
*/
export function __experimentalShowBlockInterface() {
return {
type: 'SHOW_BLOCK_INTERFACE',
};
}

/**
* Returns an action object used in signalling that the user has begun to type.
*
Expand Down Expand Up @@ -1446,45 +1424,6 @@ export function updateSettings( settings ) {
return __experimentalUpdateSettings( settings, true );
}

/**
* A list of private/experimental block editor settings that
* should not become a part of the WordPress public API.
* BlockEditorProvider will remove these settings from the
* settings object it receives.
*
* @see https://github.com/WordPress/gutenberg/pull/46131
*/
const privateSettings = [ 'inserterMediaCategories' ];

/**
* Action that updates the block editor settings and
* conditionally preserves the experimental ones.
*
* @param {Object} settings Updated settings
* @param {boolean} stripExperimentalSettings Whether to strip experimental settings.
* @return {Object} Action object
*/
export function __experimentalUpdateSettings(
settings,
stripExperimentalSettings = false
) {
let cleanSettings = settings;
// There are no plugins in the mobile apps, so there is no
// need to strip the experimental settings:
if ( stripExperimentalSettings && Platform.OS === 'web' ) {
cleanSettings = {};
for ( const key in settings ) {
if ( ! privateSettings.includes( key ) ) {
cleanSettings[ key ] = settings[ key ];
}
}
}
return {
type: 'UPDATE_SETTINGS',
settings: cleanSettings,
};
}

/**
* Action that signals that a temporary reusable block has been saved
* in order to switch its temporary id with the real id.
Expand Down
14 changes: 7 additions & 7 deletions packages/block-editor/src/store/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ import { createReduxStore, registerStore } from '@wordpress/data';
*/
import reducer from './reducer';
import * as selectors from './selectors';
import * as allActions from './actions';
import * as privateActions from './private-actions';
import * as privateSelectors from './private-selectors';
import * as actions from './actions';
import { STORE_NAME } from './constants';
import { unlock } from '../experiments';

const { __experimentalUpdateSettings, ...actions } = allActions;

/**
* Block editor data store configuration.
*
Expand All @@ -34,14 +34,14 @@ export const store = createReduxStore( STORE_NAME, {
...storeConfig,
persist: [ 'preferences' ],
} );
unlock( store ).registerPrivateActions( privateActions );
unlock( store ).registerPrivateSelectors( privateSelectors );

// We will be able to use the `register` function once we switch
// the "preferences" persistence to use the new preferences package.
const registeredStore = registerStore( STORE_NAME, {
...storeConfig,
persist: [ 'preferences' ],
} );

unlock( registeredStore ).registerPrivateActions( {
__experimentalUpdateSettings,
} );
unlock( registeredStore ).registerPrivateActions( privateActions );
unlock( registeredStore ).registerPrivateSelectors( privateSelectors );
Comment on lines +37 to +47
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guessing I need to register private action/selectors on both the store and registeredStore?

Copy link
Contributor

Choose a reason for hiding this comment

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

Locking only the registeredStore should be enough.. It didn't work for you if you did that? 🤔

65 changes: 65 additions & 0 deletions packages/block-editor/src/store/private-actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/**
* WordPress dependencies
*/
import { Platform } from '@wordpress/element';

/**
* A list of private/experimental block editor settings that
* should not become a part of the WordPress public API.
* BlockEditorProvider will remove these settings from the
* settings object it receives.
*
* @see https://github.com/WordPress/gutenberg/pull/46131
*/
const privateSettings = [ 'inserterMediaCategories' ];

/**
* Action that updates the block editor settings and
* conditionally preserves the experimental ones.
*
* @param {Object} settings Updated settings
* @param {boolean} stripExperimentalSettings Whether to strip experimental settings.
* @return {Object} Action object
*/
export function __experimentalUpdateSettings(
Copy link
Contributor Author

@talldan talldan Jan 27, 2023 •
edited

Choose a reason for hiding this comment

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

This one was introduced in #47319 as another locked action.

I've moved it to the private-actions file, which is a bit easier to handle compared to specifying every private action/selector in the index file.

(I imagine we want to remove the __experimental prefix from this one too, I don't mind doing that in a separate PR)

Copy link
Contributor

@adamziel adamziel Jan 27, 2023 •
edited

Choose a reason for hiding this comment

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

I used the __experimental prefix to differentiate it from the unprefixed updateSettings action. In theory the unlocked store could shadow the stable action with a private one with the same name, but I worry about hard to identify bugs where the private action isn't registered for some reason and you call the public one without realizing and without a clear error message. It would be good to have a separate discussion focused just on prefixing as the topic comes up in every other PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we can skip the __experimentalUpdateSettings for now and discuss/explore it separately. For newly created actions, selectors, etc.. I think it's good to skip the prefix.

settings,
stripExperimentalSettings = false
) {
let cleanSettings = settings;
// There are no plugins in the mobile apps, so there is no
// need to strip the experimental settings:
if ( stripExperimentalSettings && Platform.OS === 'web' ) {
cleanSettings = {};
for ( const key in settings ) {
if ( ! privateSettings.includes( key ) ) {
cleanSettings[ key ] = settings[ key ];
}
}
}
return {
type: 'UPDATE_SETTINGS',
settings: cleanSettings,
};
}

/**
* Hides the block interface (eg. toolbar, outline, etc.)
*
* @return {Object} Action object.
*/
export function hideBlockInterface() {
return {
type: 'HIDE_BLOCK_INTERFACE',
};
}

/**
* Shows the block interface (eg. toolbar, outline, etc.)
*
* @return {Object} Action object.
*/
export function showBlockInterface() {
return {
type: 'SHOW_BLOCK_INTERFACE',
};
}
10 changes: 10 additions & 0 deletions packages/block-editor/src/store/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* Returns true if the block interface is hidden, or false otherwise.
*
* @param {Object} state Global application state.
*
* @return {boolean} Whether the block toolbar is hidden.
*/
export function isBlockInterfaceHidden( state ) {
return state.isBlockInterfaceHidden;
}
11 changes: 0 additions & 11 deletions packages/block-editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1279,17 +1279,6 @@ export function isTyping( state ) {
return state.isTyping;
}

/**
* Returns true if the the block interface should be hidden, or false otherwise.
*
* @param {Object} state Global application state.
*
* @return {boolean} Whether the block toolbar is hidden.
*/
export function __experimentalIsBlockInterfaceHidden( state ) {
return state.isBlockInterfaceHidden;
}

/**
* Returns true if the user is dragging blocks, or false otherwise.
*
Expand Down
18 changes: 0 additions & 18 deletions packages/block-editor/src/store/test/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ const noop = () => {};

const {
clearSelectedBlock,
__experimentalHideBlockInterface: hideBlockInterface,
insertBlock,
insertBlocks,
mergeBlocks,
Expand All @@ -40,7 +39,6 @@ const {
replaceInnerBlocks,
resetBlocks,
selectBlock,
__experimentalShowBlockInterface: showBlockInterface,
showInsertionPoint,
startMultiSelect,
startTyping,
Expand Down Expand Up @@ -777,22 +775,6 @@ describe( 'actions', () => {
} );
} );

describe( 'hideBlockInterface', () => {
it( 'should return the HIDE_BLOCK_INTERFACE action', () => {
expect( hideBlockInterface() ).toEqual( {
type: 'HIDE_BLOCK_INTERFACE',
} );
} );
} );

describe( 'showBlockInterface', () => {
it( 'should return the SHOW_BLOCK_INTERFACE action', () => {
expect( showBlockInterface() ).toEqual( {
type: 'SHOW_BLOCK_INTERFACE',
} );
} );
} );

describe( 'startTyping', () => {
it( 'should return the START_TYPING action', () => {
expect( startTyping() ).toEqual( {
Expand Down
22 changes: 22 additions & 0 deletions packages/block-editor/src/store/test/private-actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* Internal dependencies
*/
import { hideBlockInterface, showBlockInterface } from '../private-actions';

describe( 'private actions', () => {
describe( 'hideBlockInterface', () => {
it( 'should return the HIDE_BLOCK_INTERFACE action', () => {
expect( hideBlockInterface() ).toEqual( {
type: 'HIDE_BLOCK_INTERFACE',
} );
} );
} );

describe( 'showBlockInterface', () => {
it( 'should return the SHOW_BLOCK_INTERFACE action', () => {
expect( showBlockInterface() ).toEqual( {
type: 'SHOW_BLOCK_INTERFACE',
} );
} );
} );
} );
24 changes: 24 additions & 0 deletions packages/block-editor/src/store/test/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* Internal dependencies
*/
import { isBlockInterfaceHidden } from '../private-selectors';

describe( 'private selectors', () => {
describe( 'isBlockInterfaceHidden', () => {
it( 'should return the true if toggled true in state', () => {
const state = {
isBlockInterfaceHidden: true,
};

expect( isBlockInterfaceHidden( state ) ).toBe( true );
Copy link
Contributor

@adamziel adamziel Jan 27, 2023 •
edited

Choose a reason for hiding this comment

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

I like to test actions and selectors by creating an actual registry and confirming the entire system is well-behaved, e.g.

	const registry = createRegistry();
	registry.register( blockEditorStore );
	
	await unlock(registry.dispatch( blockEditorStore ))
		. hideBlockInterface();

	// Check that blocks store contains the new block.
	const isHidden = 
		unlock(registry.select( blockEditorStore ))
			. isBlockInterfaceHidden();
	expect( isHidden ).toBeTruthy();

This reflects how the code is actually used and tells us when the action/selector feedback loop breaks – I find it really useful.

On the other hand, testing a selector with a mock state will only throw an error if the state property name is updated – even if the action, reducer, and the selector itself have been updated properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that approach too. It's a bit beyond the scope of this PR though, so I'll look into changing this separately.

} );

it( 'should return false if toggled false in state', () => {
const state = {
isBlockInterfaceHidden: false,
};

expect( isBlockInterfaceHidden( state ) ).toBe( false );
} );
} );
} );
19 changes: 0 additions & 19 deletions packages/block-editor/src/store/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ const {
isBlockMultiSelected,
isFirstMultiSelectedBlock,
getBlockMode,
__experimentalIsBlockInterfaceHidden: isBlockInterfaceHidden,
isTyping,
isDraggingBlocks,
getDraggedBlockClientIds,
Expand Down Expand Up @@ -2259,24 +2258,6 @@ describe( 'selectors', () => {
} );
} );

describe( 'isBlockInterfaceHidden', () => {
it( 'should return the true if toggled true in state', () => {
const state = {
isBlockInterfaceHidden: true,
};

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

it( 'should return false if toggled false in state', () => {
const state = {
isBlockInterfaceHidden: false,
};

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

describe( 'isTyping', () => {
it( 'should return the isTyping flag if the block is selected', () => {
const state = {
Expand Down