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 support for Child Blocks #6753

Merged
merged 10 commits into from
May 28, 2018
Merged

Add support for Child Blocks #6753

merged 10 commits into from
May 28, 2018

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented May 15, 2018

Description

Closes #5540. I split this out from #6346 which was mostly experimental — head there for extra context!

Adds parent which lets you restrict a block so that it is only available as a nested block.

registerBlockType( 'acme/buy-button', {
	title: 'Buy Button',
	icon: 'cart',
	category: 'common',

	// Only allow in products:
	parent: [ 'acme/product' ],

	...
} );

How has this been tested?

I wrote a test plugin which uses the new API. I then made sure that Buy Button blocks only appear when inserted into a Product block.

A quick way to test this is to paste the contents of block.js into your browser console.

Screenshots

child-blocks

Types of changes

Block registration API changes:

  • Introduces parent which lets one specify which block types a block can be nested into.

Selector changes:

  • Adds canInsertBlockType which is the source of truth for whether a block can be inserted or nested.
  • Removes the allowedBlockTypes argument from getInserterItems. This can be determined from state, now.
  • Items returned by getInserterItems will include new utility and frecency attributes. utility indicates how useful we think inserting the block will be. This lets us show contextually useful blocks at the top of the Suggested tab.
  • Deprecates getSupportedBlockTypes. getInserterItems will remove any blocks that are not supported. Alternatively, canInsertBlockType can be called directly.
  • Deprecates getFrecentInserterItems. Instead, call getInserterItems and filter/sort by frecency.

Inserter changes:

  • Both inserter and inserter-with-shortcuts have been changed to use getInserterItems and its new functionality.

@noisysocks noisysocks added [Type] Enhancement A suggestion for improvement. [Feature] Extensibility The ability to extend blocks or the editing experience labels May 15, 2018
( state, blockName, parentUID ) => [
state.blockListSettings[ parentUID ],
state.editor.present.blocksByUID[ parentUID ],
state.settings,
Copy link
Member

Choose a reason for hiding this comment

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

I think we may be more granular when passing our cache variables and use the selectors to compute the references e.g:[ getEditorSettings( state ).allowedBlockTypes, getBlockListSettings( state, parentUID )... ] this way if the state changes as long as selectors we depend on are updated we are safe :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I've updated the settings dependency to be more specific in a60dc23.

I'm not sure it's a good idea to call a selector from the second argument of createSelector. If we invalidated the cache based using getBlockListSettings( state, parentUID ) and then someone went ahead and changed getBlockListSettings to use createSelector then it would mean that our cache would never invalidate.

It looks like @aduth implemented a really good solution to this problem in the form of getDependants but we are not using version 3.0.0 of rememo yet.

I'll look into updating our version of rememo in a seperate PR so that we can clean this up.

if ( ! allowedBlockTypes || ! blockType ) {
return null;
}
export const canInsertBlockType = createSelector(
Copy link
Member

Choose a reason for hiding this comment

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

Given that the name of the selector is canInsertBlockTyle I think the selector may also need to take into consideration locking. If a CPT sets a template lock we may not be able to insert a block because of that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough! Added in c3a171a.

@mtias
Copy link
Member

mtias commented May 16, 2018

Great work here. I think we could refine the inserter UI further but we should wait for the results of the design explorations.

Thinking more about the API footprint, I think we should go with parent as the name, and split post type support to postType.

A few reasons for the first:

  • The main point of this addition is to set a relationship between blocks. The term "allowed" is too specific to the insertion mechanism, which is just one aspect of the child-parent relationship.
  • "Allowed" makes more sense as an instance specific prop for InnerBlocks but less so for a configuration aspect that's part of the block type definition.

For the second, even if I think there's merit in combining, there's enough distinction on post types to go the cautious route — easier to add support later within parent than to remove it.

@mtias mtias added this to the 2.9 milestone May 16, 2018
*
* @see selectors.getInserterItems()
*/
return orderBy( items, [ 'utility', 'frecency' ], [ 'desc', 'desc' ] );
Copy link
Member

@brandonpayton brandonpayton May 16, 2018

Choose a reason for hiding this comment

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

There are multiple components sorting inserter items by utility and frecency, and I'd like to provide the same sort in the block completer (#6067) for a consistent insertion experience. Is there a good place we could share this knowledge rather than duplicating?

Copy link
Member Author

@noisysocks noisysocks May 17, 2018

Choose a reason for hiding this comment

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

How do you feel about making it so that getInserterItems sorts by default? InserterMenu would have to disable this though so that we don't double sort.

Copy link
Member

Choose a reason for hiding this comment

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

I think that would mask our intention for the inserters to use the same default sort and introduce complexity by requiring a way to disable default sort. What do you think of exporting a default sort function from editor/components/inserter and having other inserter modules explicitly import it? Then the code would say what we mean which is "Other insertion-related components use the same default sort as the primary insertion menu".

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 made getInserterItems always sort the items in e9fbe66. It turns out that my claim that we'd have to make InserterMenu disable this was flat out wrong. Having the selector sort the items makes sense to me because:

  • It's basically always what you want to do
  • It's more performant to sort the items within createSelector where the results are cached

return count / 4;
}
};
deprecated( 'getFrecentInserterItems', {
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to be able to note that one must use getInserterItems and then sort by frecency to replace getFrecentInserterItems. I may make a PR for supporting an optional hint message to deprecated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's a little frustrating right now that you can't use English words in alternative because of how we concatenate and then internationalise the console message.

For now I've just added your suggested hint to deprecated.md in 1efb6e5.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! If you're interested, we just added support for an optional hint message in 3fd06df.

@@ -4,6 +4,9 @@ Gutenberg's deprecation policy is intended to support backwards-compatibility fo

- All components in `wp.blocks.*` are removed. Please use `wp.editor.*` instead.
- `wp.blocks.withEditorSettings` is removed. Please use the data module to access the editor settings `wp.data.select( "core/editor" ).getEditorSettings()`.
- `getInserterItems`: The `allowedBlockTypes` argument was removed and the `parentUID` argument was added.
- `getFrecentInserterItems` selector removed. Please use `getInserterItems` instead.
- `getSupportedBlocks` selector removed. Please use `canInsertBlockType` instead.
Copy link
Member

Choose a reason for hiding this comment

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

It's great to see this simplification and to be able to avoid the dance of calling getSupportedBlocks( parentUID ) to have something to pass to getInserterItems. 🙇

@noisysocks
Copy link
Member Author

noisysocks commented May 17, 2018

Thinking more about the API footprint, I think we should go with parent as the name, and split post type support to postType.

👍 renamed it to parent in b46ec6b. We'll add post type restricting in a later PR.

@tofumatt
Copy link
Member

FYI I'm out until May 28 so feel free to remove me from review if it's blocking stuff.

@noisysocks
Copy link
Member Author

How are we feeling about this? Would like to get it merged soon so that it has time to bake before 3.0.

@noisysocks noisysocks requested a review from a team May 21, 2018 01:35
@mtias
Copy link
Member

mtias commented May 21, 2018

Let's get this in shortly after doing the minor release fixes.

@aduth aduth self-requested a review May 23, 2018 14:13
* the number of inserts that have occurred.
*/
function getInsertUsage( state, id ) {
return state.preferences.insertUsage[ id ] || { time: undefined, count: 0 };
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Should we add a DEFAULT_INSERT_USAGE object equal to { time: undefined, count: 0 };? We are creating a new object each time the id is not found. If later we use this as a prop we will cause rerenders.

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'll just avoid this fanciness altogether and return null if the insert is missing 🙂 b2f8cc1

return count;
}

const duration = Date.now() - time;
Copy link
Member

Choose a reason for hiding this comment

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

We are using Date.now() but at the same time caching the selector which means "now" is not that relevant and it does not need to be now. "Now" in this case is the last time the relevant state changed. I think instead of Date.now() we can use the biggest time from all the blocks. But I'm just noting it in case later we decide to refactor the code to make sure our selectors don't rely on any external info besides the state itself. This is not something new, so I think we can keep the current approach.

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 think it's fine since "Now" in this case is the last time the relevant state changed is exactly what we want.

Using the greatest time from all of the blocks is a good idea for a pure alternative, but I don't think it's worth the performance penalty we'd get from iterating through every block.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine since "Now" in this case is the last time the relevant state changed is exactly what we want.

This would have made for a good inline code comment to the next person who maintains this selector.

Copy link
Member Author

Choose a reason for hiding this comment

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

👌 added in aec5bc1

@@ -1463,6 +1484,8 @@ describe( 'selectors', () => {
},
innerBlocks: [],
} );

unregisterBlockType( 'core/meta-block' );
Copy link
Member

Choose a reason for hiding this comment

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

This should be moved to the afterAll / afterEach method.

Copy link
Member Author

Choose a reason for hiding this comment

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

core/meta-block is only used in one it().

@@ -119,7 +138,9 @@ describe( 'selectors', () => {
} );

afterAll( () => {
unregisterBlockType( 'core/test-block' );
unregisterBlockType( 'core/test-block-a' );
Copy link
Member

Choose a reason for hiding this comment

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

Should we also unregister core/block?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, fixed in 4f9352e 👍

* @property {boolean} isDisabled Whether or not the user should be prevented from inserting
* this item.
* @property {number} utility How useful we think this item is, between 0 and 3.
* @property {number} frecency Hueristic that combines recency and frecency.
Copy link
Member

Choose a reason for hiding this comment

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

Should be rather:

Hueristic that combines recency and freqency

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦‍♂️ 99ca557

if ( ! supportedNestedBlocks ) {
return false;
}
export function getSupportedBlocks( state, uid ) {
Copy link
Member

Choose a reason for hiding this comment

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

What about globallyEnabledBlockTypes param? It is still included in JSDoc but not used in the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

getSupportedBlocks has to accept globallyEnabledBlockTypes to be backwards compatible. I removed it from the signature to avoid an unused variable linter warning. I've made this more explicit by just disabling the linter warning in d42964f 👍

@@ -7,6 +7,9 @@ Gutenberg's deprecation policy is intended to support backwards-compatibility fo
- All DOM utils in `wp.utils.*` are removed. Please use `wp.dom.*` instead.
- `isPrivate: true` has been removed from the Block API. Please use `supports.inserter: false` instead.
- `wp.utils.isExtraSmall` function removed. Please use `wp.viewport.*` instead.
- `getInserterItems`: the `allowedBlockTypes` argument was removed and the `parentUID` argument was added.
Copy link
Member

Choose a reason for hiding this comment

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

It will be scheduled for deprecation with 3.2.0 release.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 changed in 4eb77c8

if ( ! referencedBlock ) {
return null;
}
if ( ! isBlockAllowedInParent ) {
Copy link
Member

Choose a reason for hiding this comment

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

Eslint says you can do:

return isBlockAllowedInParent;

Copy link
Member

@aduth aduth May 29, 2018

Choose a reason for hiding this comment

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

Eslint says you can do:

return isBlockAllowedInParent;

This was never addressed.

I'd go further and say the isBlockAllowedInParent variable is unnecessary. We could just return at the point where it is assigned above. I don't even think an argument of it being an explaining variable is valid, since its meaning is implied as being the return value of the function (i.e. encapsulated already in the function naming).

Copy link
Member Author

Choose a reason for hiding this comment

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

👌 changed in bbe03fe

function fillWithCommonBlocks( inserts ) {
// Filter out any inserts that are associated with a block type that isn't registered
const items = inserts.filter( ( insert ) => getBlockType( insert.name ) );
if ( ! canInsertBlockType( state, blockType.name, parentUID ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Apparently, Eslint doesn't like your explicit return statements with boolean values :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Where are you seeing these? When I run npm run lint there's no warnings 😕

(Also, I like the explicitness…)

Copy link
Member

Choose a reason for hiding this comment

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

I disagree here, as it seems to imply we expect the result of canInsertBlockType to be anything other than a boolean.

  • If we do expect it to be a boolean, why not just return it as-is?
  • If we don't expect it to be a boolean, is a "can" prefix a good choice of naming?

Copy link
Member Author

Choose a reason for hiding this comment

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

👌 changed in fac2869

{
id: 'core/block/123',
id: 'core/test-block-b',
Copy link
Member

Choose a reason for hiding this comment

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

When looking only at this test it is unclear why Block B is listed before Block A. utility value inherited from the common category does the trick. Maybe Block B should be removed from this test and included in new test which tests only order of the items based on utility and frecency?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 changed in 470d32e

] );
const items = getInserterItems( state );
const testBlockBItem = items.find( ( item ) => item.id === 'core/test-block-b' );
expect( testBlockBItem.utility ).toBe( 1 );
Copy link
Member

Choose a reason for hiding this comment

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

Should we use constant MEDIUM instead of value 1 to better match with the description of the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 added some constants in 6efa3af

};
const items = getInserterItems( state );
const sharedBlock2Item = items.find( ( item ) => item.id === 'core/test-block-b' );
expect( sharedBlock2Item.utility ).toBe( 2 );
Copy link
Member

Choose a reason for hiding this comment

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

Value 2 is hard to follow in here, the description says high.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 added some constants in 6efa3af

@gziolo
Copy link
Member

gziolo commented May 23, 2018

This PR needs rebase, also description is out of the date after allowedParentBlocks was renamed to parent :)

} = select( 'core/editor' );
const { allowedBlockTypes, templateLock } = getEditorSettings();
const { templateLock } = getEditorSettings();
Copy link
Member

Choose a reason for hiding this comment

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

Minor: We can now remove the usage of template lock as getInserterItems if there is a lock getInserterItems will return no items.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 4f6e662 👌

items: getInserterItems( supportedBlocks ),
frecentItems: getFrecentInserterItems( supportedBlocks ),
};
} ),
withDispatch( ( dispatch ) => ( {
fetchSharedBlocks: dispatch( 'core/editor' ).fetchSharedBlocks,
Copy link
Member

Choose a reason for hiding this comment

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

Minor: It is a little strange that we are dispatching a fetch action here but we are not selecting anything in this component. To make this relationship more clear maybe the items: getInserterItems( rootUid ) should continue to be done here and not passed as prop.

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'll leave this for now because we're looking into replacing this action with a resolver which would address this.

Copy link
Member

Choose a reason for hiding this comment

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

I'll leave this for now because we're looking into replacing this action with a resolver which would address this.

Where is this tracked?

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 count * 4;
case duration < ( 24 * 3600 ):
return count * 2;
case duration < ( 7 * 24 * 3600 ):
Copy link
Member

Choose a reason for hiding this comment

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

minor: Maybe we can create constants at the top e.g: SECONDS_IN_A_WEEK = 7 * SECONDS_IN_A_DAY.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 7e215a0 👌

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Manually testing this PR, it looks good to me, I left some non-blocker code improvements.
We will continue to iterate on this, e.g., apply restrictions on autocomplete and transforms, add locking to nesting, etc. So during the next steps, if we find a possible improvement, we can do it in the follow-up PRs.
I think we can merge now, so we have more time to test before the release.
Good work @noisysocks!

Block registration API changes:

- Introduces `parent` which lets one specify which block types a block
  can be nested into.

Selector changes:

- Adds `canInsertBlockType` which is the source of truth for whether a
  block can be inserted or nested.
- Removes the `allowedBlockTypes` argument from `getInserterItems`. This
  can be determined from state, now.
- Items returned by `getInserterItems` will include new `utility` and
  `frecency` attributes. `utility` indicates how useful we think inserting
  the block will be. This lets us show contextually useful blocks at the
  top of the Suggested tab.
- Deprecates `getSupportedBlockTypes`. `getInserterItems` will remove
  any blocks that are not supported. Alternatively, `canInsertBlockType`
  can be called directly.
- Deprecates `getFrecentInserterItems`. Instead, call `getInserterItems`
  and filter/sort by `frecency`.

Inserter changes:

- Both `inserter` and `inserter-with-shortcuts` have been changed to use
  `getInserterItems` and its new functionality.
getInserterItems() will check whether or not the template is locked, so
we don't need to do it twice.
Make calculateFrecency() more readable by using SECONDS_PER_* constants
instead of magic numbers.
@noisysocks noisysocks merged commit bdcbf71 into master May 28, 2018
@noisysocks noisysocks deleted the add/child-blocks branch May 28, 2018 01:39
@noisysocks
Copy link
Member Author

Thanks all! 🎉❤️


* **Type:** `Array`

Blocks can be inserted into other blocks as nested content. Sometimes it is useful to restrict a block so that it is only available as a nested block. For example, you might want to allow an 'Add to Cart' block to only be available within a 'Product' block.
Copy link
Member

Choose a reason for hiding this comment

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

Blocks can be inserted into other blocks as nested content

By the current wording, there's an implication that I could insert a block into e.g. an image block, which is not true. This is to say: Not all blocks support nested content.

Copy link
Member Author

Choose a reason for hiding this comment

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

👌 changed in 920d7cc

items: getInserterItems( supportedBlocks ),
frecentItems: getFrecentInserterItems( supportedBlocks ),
};
} ),
withDispatch( ( dispatch ) => ( {
fetchSharedBlocks: dispatch( 'core/editor' ).fetchSharedBlocks,
Copy link
Member

Choose a reason for hiding this comment

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

I'll leave this for now because we're looking into replacing this action with a resolver which would address this.

Where is this tracked?

* Each item object contains what's necessary to display a button in the
* inserter and handle its selection.
*
* The 'utility' property indicates how useful we think an item will be to the
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the difference between utility as defined here and priority as defined by @wordpress/hooks and the block type transforms property?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure that I understand your question, sorry. Would you prefer that I rename utilitypriority? I named it utility because it's a measure of how useful an inserter item is, but I'm not tied to the name.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure that I understand your question, sorry. Would you prefer that I rename utilitypriority? I named it utility because it's a measure of how useful an inserter item is, but I'm not tied to the name.

I'm curious more in hopes of minimizing the number of concepts which exist, where an existing pattern already exists to serve a need. As I read further, I see that these are used as specific multipliers, so while similar to priority, not quite the same.

Which is to say: Nothing should need to be changed here.

Copy link
Member

Choose a reason for hiding this comment

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

At some point we might want to consolidate those two properties: utility and frecency into one property called score. This is how some search engines work. The score could be calculated using weighted sum model to translate how things are sorted at the moment.

if ( ! referencedBlock ) {
return null;
}
if ( ! isBlockAllowedInParent ) {
Copy link
Member

@aduth aduth May 29, 2018

Choose a reason for hiding this comment

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

Eslint says you can do:

return isBlockAllowedInParent;

This was never addressed.

I'd go further and say the isBlockAllowedInParent variable is unnecessary. We could just return at the point where it is assigned above. I don't even think an argument of it being an explaining variable is valid, since its meaning is implied as being the return value of the function (i.e. encapsulated already in the function naming).

( state, blockName, parentUID ) => [
state.blockListSettings[ parentUID ],
state.editor.present.blocksByUID[ parentUID ],
state.settings.allowedBlockTypes,
Copy link
Member

Choose a reason for hiding this comment

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

Aside: This pierces the abstraction of getEditorSettings in use by the selector. Not an easy problem to solve, though.

return count;
}

const duration = Date.now() - time;
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine since "Now" in this case is the last time the relevant state changed is exactly what we want.

This would have made for a good inline code comment to the next person who maintains this selector.

function fillWithCommonBlocks( inserts ) {
// Filter out any inserts that are associated with a block type that isn't registered
const items = inserts.filter( ( insert ) => getBlockType( insert.name ) );
if ( ! canInsertBlockType( state, blockType.name, parentUID ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

I disagree here, as it seems to imply we expect the result of canInsertBlockType to be anything other than a boolean.

  • If we do expect it to be a boolean, why not just return it as-is?
  • If we don't expect it to be a boolean, is a "can" prefix a good choice of naming?


let isDisabled = false;
if ( blockType.useOnce ) {
isDisabled = getBlocks( state ).some( ( block ) => block.name === blockType.name );
Copy link
Member

Choose a reason for hiding this comment

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

FYI: Lodash's _.some has a nice convenience for this pattern in its object predicates:

isDisabled = some( getBlocks( state ), { name: blockType.name } );

https://lodash.com/docs/4.17.10#some

Copy link
Member Author

Choose a reason for hiding this comment

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

👌 changed in ed0b4f1


const duration = Date.now() - time;
switch ( true ) {
case duration < SECONDS_PER_HOUR:
Copy link
Member

Choose a reason for hiding this comment

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

Date.now returns a value measured in milliseconds. Is it correct that these constants are measured in seconds?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/now

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch! Fixed in c3dd54b

@@ -1597,24 +1720,18 @@ export function getBlockListSettings( state, uid ) {
*
* @return {string[]|boolean} Blocks that can be nested inside the block with the specified uid, or true/false to enable/disable all types.
*/
// Disable reason: We have to accept `globallyEnabledBlockTypes` so as to be backwards compatible
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand. Can you explain what will break if globallyEnabledBlockTypes were removed as an argument here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing will break, but it masks the fact that this method accepts globallyEnabledBlockTypes as its third argument and must continue to do until it is removed in 3.2.

For example, say that we remove the argument. Now, imagine that a developer wants to add a new sortBy argument in version 3.0. They would likely change the signature to look like this:

function getSupportedBlocks( state, uid, sortBy = 'asc' )

The developer assumes that this is a non-breaking change, since they are just adding a new optional argument. But this is incorrect, since it will break any pre-3.0 code that makes calls such as wp.data.select( 'core/editor' ).getSupportedBlocks( 'abc123', [ 'core/paragraph' ] ).

Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable. My question was mostly for...

  • Ensuring we don't keep dead code around (the reason the rule exists in the first place)
  • Clarifying wording, since it implies that it is necessary for some reason, though it is not used ("We have to accept...")

@tomasruud
Copy link

Are there any plans on making it possible to fetch nested block contents from a dynamic block?

@aduth
Copy link
Member

aduth commented Jun 19, 2018

@tomasruud The implementation of do_blocks (front-end block processing) is currently incapable of supporting nesting within dynamic blocks, since it does not consider tag balancing for dynamic block markers.

See also: #4591 (comment)

return null;
}
export const canInsertBlockType = createSelector(
( state, blockName, parentUID = null ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Was there a reason we named this with a "parent" prefix, unlike "root" we use everywhere else?

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 suppose it should be renamed for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants