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

[WIP] Block Editor Store: Allow management of block inspector tabs #46271

Closed
wants to merge 1 commit into from
Closed

[WIP] Block Editor Store: Allow management of block inspector tabs #46271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 43 additions & 0 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 @@ -1748,3 +1748,46 @@ export function __unstableSetTemporarilyEditingAsBlocks(
temporarilyEditingAsBlocks,
};
}

/**
* Action used to enable display of block inspector tabs overriding the
* Gutenberg block inspector tabs experiment setting.
*
* @param {?string} blockName The block's name.
*/
export function __experimentalEnableBlockInspectorTabs( blockName ) {
return {
type: 'ENABLE_BLOCK_INSPECTOR_TABS',
blockName,
};
}

/**
* Action used to disable display of block inspector tabs overriding the
* Gutenberg block inspector tabs experiment setting.
*
* @param {?string} blockName The block's name.
*/
export function __experimentalDisableBlockInspectorTabs( blockName ) {
return {
type: 'DISABLE_BLOCK_INSPECTOR_TABS',
blockName,
};
}

/**
* Action to set default tab for a specific block type.
*
* @param {string} blockName The block's name.
* @param {string} defaultTab Name of tab to display by default.
*/
export function __experimentalSetDefaultBlockInspectorTab(
blockName,
defaultTab
) {
return {
type: 'SET_DEFAULT_BLOCK_INSPECTOR_TAB',
blockName,
defaultTab,
};
}
50 changes: 50 additions & 0 deletions packages/block-editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1842,6 +1842,55 @@ export function temporarilyEditingAsBlocks( state = '', action ) {
return state;
}

/**
* Reducer returning the current configuration for block inspector tabs.
*
* @param {Object} state Current configuration state for block inspector tabs.
* @param {Object} action Dispatched action.
*
* @return {Object} Updated state.
*/
export function blockInspectorTabs( state = {}, action ) {
Copy link
Contributor

@talldan talldan Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It's an interesting one, because reducers are usually used for values that are likely to change at runtime. I wonder if there will be a use case for changing these values while the editor is running.

My feeling is that these values will probably be set as an initial configuration of the editor, which is often the job of the block editor settings (although technically they can change at runtime too, but often don't).

I think the only pain point will be that it's a little more difficult to handle deeply nested objects, but I think the data structure could be simplified (as mentioned in my other comment).

Editor settings can also be set in PHP, so that another advantage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to note we have a use case for the Nav offcanvas experiment whereby we'd need to programmatically select the default tab as a result of an interaction with the "edit" button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appreciate the feedback 👍

because reducers are usually used for values that are likely to change at runtime

The desire to set the default tab for the Navigation Link block, depending on if it was edited via the parent Navigation block's list view, drove the need for these values to change at runtime.

My feeling is that these values will probably be set as an initial configuration of the editor, which is often the job of the block editor settings

Splitting the management of the block inspector tab display so the default tabs are handled separately would open up the possibility of using the block editor settings. I'll be happy to explore that approach as well.

In the meantime, I wonder if this is reason enough to reconsider disabling tabs entirely for the Navigation Link block in the short term. It would allow us to avoid adding anything to the store related to the tabs or navigation experiments and all tab management settings would still be contained in a single location.

Copy link
Contributor

@talldan talldan Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just to note we have a use case for the Nav offcanvas experiment whereby we'd need to programmatically select the default tab as a result of an interaction with the "edit" button.

@getdave Aaron explained this to me in a private message. My mistake for not reading the PR description thoroughly.

The main problem is that I don't think a stable API should be introduced for two experimental features. There is a proposal for experimental selectors/actions, which would help:

I don't know if the __experimental prefix is still allowed? cc @adamziel

Alternatively the prefix could be used, but the selectors/actions would need to be stabilized or removed for alternatives before 6.2, so that they're not released with WordPress core.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the meantime, I wonder if this is reason enough to reconsider disabling tabs entirely for the Navigation Link block in the short term. It would allow us to avoid adding anything to the store related to the tabs or navigation experiments and all tab management settings would still be contained in a single location.

@scruffian If we go down this route then will this compromise the user testing of the Nav offcanvas experiment?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think as long as we can put the link settings above the color ones then this should be an ok interim solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is what the navigation link block currently looks like with the tabs disabled. The label input is the first much like I imagine is desired.

Screenshot 2022-12-05 at 6 19 36 pm

Copy link
Contributor

@adamziel adamziel Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know if the __experimental prefix is still allowed? cc @adamziel

Once the tooling is merged it will be safe to stop introducing new prefixed APIs entirely, but we're not there yet. Ideally there wouldn't be anything __experimental in public exports, but at the moment we cant avoid it sometimes.

switch ( action.type ) {
case 'ENABLE_BLOCK_INSPECTOR_TABS':
if ( ! action.blockName ) {
return { ...state, enabled: true };
}

return {
...state,
[ action.blockName ]: {
...state?.[ action.blockName ],
enabled: true,
},
};

case 'DISABLE_BLOCK_INSPECTOR_TABS':
if ( ! action.blockName ) {
return { ...state, enabled: false };
}

return {
...state,
[ action.blockName ]: {
...state?.[ action.blockName ],
enabled: false,
},
};

case 'SET_DEFAULT_BLOCK_INSPECTOR_TAB':
return {
...state,
[ action.blockName ]: {
...state?.[ action.blockName ],
defaultTab: action.defaultTab,
},
};
Comment on lines +1881 to +1888
Copy link
Contributor

Choose a reason for hiding this comment

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

The default tab could be a separate reducer, and it'd simplify the code quite a bit. DISABLE_BLOCK_INSPECTOR_TABS could be simplified to adding a block name to a unique array (so that the state is something like this disabledInspectorTabs: [ 'core/freeform', ... ]).

SET_DEFAULT_BLOCK_INSPECTOR_TAB could be storing a map of block names to tab names (defaultInspectorTabs: { 'core/navigation': 'menu', ... }).

Or you could use combineReducers to have nested state (e.g. with this shape - inspectorTabs: { disabled: [ 'core/freeform' ], defaultTab: { 'core/navigation': 'menu' } }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for highlighting this.

DISABLE_BLOCK_INSPECTOR_TABS could be simplified to adding a block name to a unique array (so that the state is something like this disabledInspectorTabs: [ 'core/freeform', ... ]).

I think we'll need to tweak the proposed structure a little further to allow for overriding the default display of tabs for all blocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is an all block setting definitely needed?

I do wonder if that could be explored as a user preference (especially as an accessibility option), but worth taking this one challenge at a time.

Copy link
Contributor Author

@aaronrobertshaw aaronrobertshaw Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

During some recent discussions, it was requested that plugins be able to disable/enable tabs across all blocks but that it shouldn't necessarily be something controlled by end users.

I'll have another think about it. It could also be something we expanded upon in the future.

}

return state;
}

export default combineReducers( {
blocks,
isTyping,
Expand All @@ -1865,4 +1914,5 @@ export default combineReducers( {
lastBlockInserted,
temporarilyEditingAsBlocks,
blockVisibility,
blockInspectorTabs,
} );
27 changes: 27 additions & 0 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 @@ -2766,3 +2766,30 @@ export function __unstableIsWithinBlockOverlay( state, clientId ) {
}
return false;
}

/**
* Returns true if the block inspector should display tabs, false otherwise.
*
* @param {Object} state Global application state.
* @param {?string} blockName The block type name.
*
* @return {boolean} Whether block inspector tabs should be shown or not.
*/
export function __experimentalAreBlockInspectorTabsEnabled( state, blockName ) {
const enabled = state.blockInspectorTabs?.[ blockName ]?.enabled;

return enabled === undefined ? state.blockInspectorTabs?.enabled : enabled;
}

/**
* Retrieves the default block inspector tab's name for the block type, if
* available.
*
* @param {Object} state Global application state.
* @param {string} blockName The block type name.
*
* @return {string|undefined} The name of the default block inspector tab if set.
*/
export function __experimentalGetDefaultBlockInspectorTab( state, blockName ) {
return state.blockInspectorTabs?.[ blockName ]?.defaultTab;
}
45 changes: 45 additions & 0 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,6 +27,8 @@ const noop = () => {};

const {
clearSelectedBlock,
__experimentalDisableBlockInspectorTabs: disableBlockInspectorTabs,
__experimentalEnableBlockInspectorTabs: enableBlockInspectorTabs,
__experimentalHideBlockInterface: hideBlockInterface,
insertBlock,
insertBlocks,
Expand All @@ -40,6 +42,7 @@ const {
replaceInnerBlocks,
resetBlocks,
selectBlock,
__experimentalSetDefaultBlockInspectorTab: setDefaultBlockInspectorTab,
__experimentalShowBlockInterface: showBlockInterface,
showInsertionPoint,
startMultiSelect,
Expand Down Expand Up @@ -793,6 +796,48 @@ describe( 'actions', () => {
} );
} );

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

it( 'should return the ENABLE_BLOCK_INSPECTOR_TABS action for specific block', () => {
expect( enableBlockInspectorTabs( 'core/foo' ) ).toEqual( {
type: 'ENABLE_BLOCK_INSPECTOR_TABS',
blockName: 'core/foo',
} );
} );
} );

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

it( 'should return the DISABLE_BLOCK_INSPECTOR_TABSDISABLE_BLOCK_INSPECTOR_TABS action for specific block', () => {
expect( disableBlockInspectorTabs( 'core/foo' ) ).toEqual( {
type: 'DISABLE_BLOCK_INSPECTOR_TABS',
blockName: 'core/foo',
} );
} );
} );

describe( 'setDefaultBlockInspectorTab', () => {
it( 'should return SET_DEFAULT_BLOCK_INSPECTOR_TAB action', () => {
expect(
setDefaultBlockInspectorTab( 'core/foo', 'settings' )
).toEqual( {
type: 'SET_DEFAULT_BLOCK_INSPECTOR_TAB',
blockName: 'core/foo',
defaultTab: 'settings',
} );
} );
} );

describe( 'startTyping', () => {
it( 'should return the START_TYPING action', () => {
expect( startTyping() ).toEqual( {
Expand Down
73 changes: 73 additions & 0 deletions packages/block-editor/src/store/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
blockListSettings,
lastBlockAttributesChange,
lastBlockInserted,
blockInspectorTabs,
} from '../reducer';

const noop = () => {};
Expand Down Expand Up @@ -2360,6 +2361,78 @@ describe( 'state', () => {
} );
} );

describe( 'blockInspectorTabs()', () => {
it( 'should enable inspector tabs for all blocks', () => {
const state = blockInspectorTabs(
{ enabled: false },
{ type: 'ENABLE_BLOCK_INSPECTOR_TABS' }
);

expect( state ).toEqual( { enabled: true } );
} );

it( 'should enable inspector tabs for a single block', () => {
const state = blockInspectorTabs(
{ enabled: false },
{
type: 'ENABLE_BLOCK_INSPECTOR_TABS',
blockName: 'core/block',
}
);

expect( state ).toEqual( {
enabled: false,
'core/block': { enabled: true },
} );
} );

it( 'should disable inspector tabs for all blocks', () => {
const state = blockInspectorTabs(
{ enabled: true },
{ type: 'DISABLE_BLOCK_INSPECTOR_TABS' }
);

expect( state ).toEqual( { enabled: false } );
} );

it( 'should disable inspector tabs for a single block', () => {
const state = blockInspectorTabs(
{ enabled: true },
{
type: 'DISABLE_BLOCK_INSPECTOR_TABS',
blockName: 'core/block',
}
);

expect( state ).toEqual( {
enabled: true,
'core/block': { enabled: false },
} );
} );

it( 'should set the default inspector tab for a single block', () => {
const state = blockInspectorTabs(
{
enabled: false,
'core/block': { enabled: true },
},
{
type: 'SET_DEFAULT_BLOCK_INSPECTOR_TAB',
blockName: 'core/block',
defaultTab: 'settings',
}
);

expect( state ).toEqual( {
enabled: false,
'core/block': {
enabled: true,
defaultTab: 'settings',
},
} );
} );
} );

describe( 'isTyping()', () => {
it( 'should set the typing flag to true', () => {
const state = isTyping( false, {
Expand Down
63 changes: 63 additions & 0 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 @@ -73,6 +73,8 @@ const {
__experimentalGetPatternTransformItems,
wasBlockJustInserted,
__experimentalGetGlobalBlocksByName,
__experimentalAreBlockInspectorTabsEnabled: areBlockInspectorTabsEnabled,
__experimentalGetDefaultBlockInspectorTab: getDefaultBlockInspectorTab,
} = selectors;

describe( 'selectors', () => {
Expand Down Expand Up @@ -2084,6 +2086,67 @@ describe( 'selectors', () => {
} );
} );

describe( 'areBlockInspectorTabsEnabled', () => {
it( 'should not provide default if not set in state', () => {
const state = {};

expect( areBlockInspectorTabsEnabled( state ) ).toBe( undefined );
} );

it( 'should return true when toggled true in state', () => {
const state = {
blockInspectorTabs: { enabled: true },
};

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

it( 'should return false when toggled false in state', () => {
const state = {
blockInspectorTabs: { enabled: false },
};

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

it( 'should return block specific override for tabs display', () => {
const state = {
blockInspectorTabs: {
enable: false,
'core/block': { enabled: true },
},
};

expect( areBlockInspectorTabsEnabled( state, 'core/block' ) ).toBe(
true
);
} );

it( 'should fallback to default value when block not specifically set', () => {
const state = {
blockInspectorTabs: { enabled: true },
};

expect( areBlockInspectorTabsEnabled( state, 'core/block' ) ).toBe(
true
);
} );
} );

describe( 'getDefaultBlockInspectorTab', () => {
it( 'should return default tab for block when set', () => {
const state = {
blockInspectorTabs: {
'core/block': { defaultTab: 'settings' },
},
};

expect( getDefaultBlockInspectorTab( state, 'core/block' ) ).toBe(
'settings'
);
} );
} );

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