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

Fix: Block manager doesn't respect allowed_block_types hook #16586

Merged
merged 3 commits into from
Aug 5, 2019
Merged

Fix: Block manager doesn't respect allowed_block_types hook #16586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff6fcbe
FIx: Block manager doesn't respect allowed_block_types hook
jorgefilipecosta Jul 15, 2019
0b98186
Add end 2 end test
jorgefilipecosta Jul 16, 2019
d06885d
Update useMemo dependencies
jorgefilipecosta Jul 26, 2019
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
25 changes: 25 additions & 0 deletions packages/e2e-tests/specs/plugins/allowed-blocks.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,29 @@ describe( 'Allowed Blocks Filter', () => {
) )[ 0 ];
expect( galleryBlockButton ).toBeUndefined();
} );

it( 'should remove not allowed blocks from the block manager', async () => {
const BLOCK_LABEL_SELECTOR = '.edit-post-manage-blocks-modal__checklist-item .components-checkbox-control__label';
await page.click(
'.edit-post-more-menu [aria-label="More tools & options"]'
);
const [ button ] = await page.$x(
`//button[contains(text(), 'Block Manager')]`
);
await button.click( 'button' );

await page.waitForSelector( BLOCK_LABEL_SELECTOR );
const blocks = await page.evaluate(
( selector ) => {
return Array.from( document.querySelectorAll( selector ) ).map(
( element ) => ( ( element.innerText || '' ).trim() )
).sort();
},
BLOCK_LABEL_SELECTOR
);
expect( blocks ).toEqual( [
'Image',
'Paragraph',
] );
} );
} );
7 changes: 7 additions & 0 deletions packages/edit-post/src/components/edit-post-settings/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/**
* WordPress dependencies
*/
import { createContext } from '@wordpress/element';
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is more for convenience in making the settings available to the modal component? I thought whether we had this tracked in state, but I guess that's only in the editor and block-editor modules. We could do the same in this module as well, but it might be a bit overkill for the one setting we're interested in (I also feel it's already become difficult to manage in the two modules it is present).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I used context as a convenience. Adding it to the edit-post store seemed overkill, and it seemed it would just bring more confusion, I guess we can reconsider later if we have other reasons to add it. While if we add now, I think we can not easily remove them because of o back-compatibility.


const EditPostSettings = createContext( {} );
export default EditPostSettings;
26 changes: 21 additions & 5 deletions packages/edit-post/src/components/manage-blocks-modal/category.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
/**
* External dependencies
*/
import { without, map } from 'lodash';
import { includes, map, without } from 'lodash';

/**
* WordPress dependencies
*/
import { useContext, useMemo } from '@wordpress/element';
import { withSelect, withDispatch } from '@wordpress/data';
import { compose, withInstanceId } from '@wordpress/compose';
import { CheckboxControl } from '@wordpress/components';
Expand All @@ -14,6 +15,7 @@ import { CheckboxControl } from '@wordpress/components';
* Internal dependencies
*/
import BlockTypesChecklist from './checklist';
import EditPostSettings from '../edit-post-settings';

function BlockManagerCategory( {
instanceId,
Expand All @@ -23,18 +25,32 @@ function BlockManagerCategory( {
toggleVisible,
toggleAllVisible,
} ) {
if ( ! blockTypes.length ) {
const settings = useContext( EditPostSettings );
const { allowedBlockTypes } = settings;
Copy link
Contributor

Choose a reason for hiding this comment

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

These are all available in the editor store why not just use that store instead of adding a new context provider?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @youknowriad, when a user disables a block in the block manager the block is removed from the editor store allowed blocks. Using just the editor store, it is not possible to differentiate between a block disabled by the user and a block restricted on the server using a plugin. In this case, we need to distinguish both cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's minor but It feels like something that can be solved in the store somehow instead of adding a new context for sharing data while we already have a context for that (the store).

const filteredBlockTypes = useMemo(
() => {
if ( allowedBlockTypes === true ) {
return blockTypes;
}
return blockTypes.filter( ( { name } ) => {
return includes( allowedBlockTypes || [], name );
} );
},
[ allowedBlockTypes, blockTypes ]
);

if ( ! filteredBlockTypes.length ) {
return null;
}

const checkedBlockNames = without(
map( blockTypes, 'name' ),
map( filteredBlockTypes, 'name' ),
...hiddenBlockTypes
);

const titleId = 'edit-post-manage-blocks-modal__category-title-' + instanceId;

const isAllChecked = checkedBlockNames.length === blockTypes.length;
const isAllChecked = checkedBlockNames.length === filteredBlockTypes.length;

let ariaChecked;
if ( isAllChecked ) {
Expand All @@ -59,7 +75,7 @@ function BlockManagerCategory( {
label={ <span id={ titleId }>{ category.title }</span> }
/>
<BlockTypesChecklist
blockTypes={ blockTypes }
blockTypes={ filteredBlockTypes }
value={ checkedBlockNames }
onItemChange={ toggleVisible }
/>
Expand Down
39 changes: 21 additions & 18 deletions packages/edit-post/src/editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
import preventEventDiscovery from './prevent-event-discovery';
import Layout from './components/layout';
import EditorInitialization from './components/editor-initialization';
import EditPostSettings from './components/edit-post-settings';

class Editor extends Component {
constructor() {
Expand Down Expand Up @@ -93,24 +94,26 @@ class Editor extends Component {

return (
<StrictMode>
<SlotFillProvider>
<DropZoneProvider>
<EditorProvider
settings={ editorSettings }
post={ post }
initialEdits={ initialEdits }
useSubRegistry={ false }
{ ...props }
>
<ErrorBoundary onError={ onError }>
<EditorInitialization postId={ postId } />
<Layout />
<KeyboardShortcuts shortcuts={ preventEventDiscovery } />
</ErrorBoundary>
<PostLockedModal />
</EditorProvider>
</DropZoneProvider>
</SlotFillProvider>
<EditPostSettings.Provider value={ settings }>
<SlotFillProvider>
<DropZoneProvider>
<EditorProvider
settings={ editorSettings }
post={ post }
initialEdits={ initialEdits }
useSubRegistry={ false }
{ ...props }
>
<ErrorBoundary onError={ onError }>
<EditorInitialization postId={ postId } />
<Layout />
<KeyboardShortcuts shortcuts={ preventEventDiscovery } />
</ErrorBoundary>
<PostLockedModal />
</EditorProvider>
</DropZoneProvider>
</SlotFillProvider>
</EditPostSettings.Provider>
</StrictMode>
);
}
Expand Down