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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try a toolbar that affects multiple selected blocks #7635

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@notnownikki
Member

notnownikki commented Jun 29, 2018

Description

Similar PRs: #1811, #3535.

This adds a new controls option in a block's settings so the toolbar can be reused when multiple blocks of the same type are selected, and have the Edit component render them automatically for individual blocks.

Adds multi block controls and higher order components to deal with multiple block selections, so block developers can define their multi block aware controls in the edit component, and have them used to both single and multiple blocks.

Why a new option in the settings?

I wanted to have an easy way for block developers to define a toolbar and use it in their block, and also have it used by block-list/multi-controls to render a toolbar that can affect multiple blocks. It's optional, so maintains backward compatibility with existing block settings.

There isn't any more! It doesn't change the API.

Why is it just a standalone function?

So that the block can pass in its own props and state if the toolbar needs it, and the multi controls can create props with attributes reduced from the selected blocks.

It's not! It allows you to define a set of multi block aware components in edit and have them reused for single block instances too.

This approach should work for the Inspector controls.

Why does it look so bad?

Because. Help! 馃槃

It doesn't now!

Works though, doesn't it?

Yep.

@notnownikki notnownikki requested review from youknowriad, mtias and iseulde Jun 29, 2018

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Jul 2, 2018

Member

@youknowriad I've moved toolbar to controls and if the block settings define controls, then the Edit component automatically renders them, so block authors don't have to include them in their edit component's render, and the components are reused for the multi block selection.

Member

notnownikki commented Jul 2, 2018

@youknowriad I've moved toolbar to controls and if the block settings define controls, then the Edit component automatically renders them, so block authors don't have to include them in their edit component's render, and the components are reused for the multi block selection.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Jul 2, 2018

Member

And now the multi-block toolbar shows the number of selected blocks, as @mtias originally requested :)

Member

notnownikki commented Jul 2, 2018

And now the multi-block toolbar shows the number of selected blocks, as @mtias originally requested :)

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Jul 3, 2018

Member

See #1811 and #3535 for previous discussion.

One of the problems with having a toolbar inside the first block wrapper is that it cannot be made sticky for the whole selection. I'm not sure what the solution here could be other than wrapping the selection in a div.

There's also the area in the block inspector which this PR could implement:

screen shot 2018-07-03 at 13 13 02

Member

iseulde commented Jul 3, 2018

See #1811 and #3535 for previous discussion.

One of the problems with having a toolbar inside the first block wrapper is that it cannot be made sticky for the whole selection. I'm not sure what the solution here could be other than wrapping the selection in a div.

There's also the area in the block inspector which this PR could implement:

screen shot 2018-07-03 at 13 13 02

Show outdated Hide outdated core-blocks/paragraph/index.js Outdated
@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Jul 11, 2018

Member

@aduth so, I've tried something slightly different :)

Outside of settings, I've defined controls which has a toolbar property. I think this solves the issue of mixing instance and non-instance code within the settings. It's backward compatible, so if you don't define it and have BlockControls in your edit component, that'll still work.

Member

notnownikki commented Jul 11, 2018

@aduth so, I've tried something slightly different :)

Outside of settings, I've defined controls which has a toolbar property. I think this solves the issue of mixing instance and non-instance code within the settings. It's backward compatible, so if you don't define it and have BlockControls in your edit component, that'll still work.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Jul 16, 2018

Member

Update after some discussion with the core team : Adding toolbar and inspector to the API has more implications than I thought, other projects consuming the block API would need to revisit what they're doing and at this stage, we want to avoid that.

I'm going to carry on with an experiment to see if block toolbar controls can be multi-block aware, so a block developer doesn't have to think about anything more than putting controls as children of a BlockControls component. I'll start off by doing this with AlignmentToolbar and if it works, it should be an approach that works for the inspector too, without changing the block API.

Member

notnownikki commented Jul 16, 2018

Update after some discussion with the core team : Adding toolbar and inspector to the API has more implications than I thought, other projects consuming the block API would need to revisit what they're doing and at this stage, we want to avoid that.

I'm going to carry on with an experiment to see if block toolbar controls can be multi-block aware, so a block developer doesn't have to think about anything more than putting controls as children of a BlockControls component. I'll start off by doing this with AlignmentToolbar and if it works, it should be an approach that works for the inspector too, without changing the block API.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Jul 17, 2018

Member

New shiny code with a new approach! All changed, rebased, and squashed.

This is a way of having multi block aware controls without having to repeat control definitions, or change the API.

A new HOC, withMultiBlockSupport can add multi block support to controls that deal with block attribute changes. The decorated controls become able to deal with single blocks, or apply changes to multiple blocks.

Block developers are able to specify controls that apply to single blocks only using BlockControls and put controls that can apply to multiple blocks in MultiBlockControls.

Example from the paragraph block:

<MultiBlockControls>
	<MultiBlockAlignmentToolbar
		value={ align }
		onChange={ ( nextAlign ) => {
			setAttributes( { align: nextAlign } );
		} }
	/>
</MultiBlockControls>
<BlockControls>
	... block specific controls would go here ...
</BlockControls>

In this example, the alignment toolbar will be used for single blocks and multiple blocks, there's no need to define it twice. The new MultiBlockControls slot will be used for both, and the alignment toolbar knows if it should be operating on a single block or the list of selected blocks.

Member

notnownikki commented Jul 17, 2018

New shiny code with a new approach! All changed, rebased, and squashed.

This is a way of having multi block aware controls without having to repeat control definitions, or change the API.

A new HOC, withMultiBlockSupport can add multi block support to controls that deal with block attribute changes. The decorated controls become able to deal with single blocks, or apply changes to multiple blocks.

Block developers are able to specify controls that apply to single blocks only using BlockControls and put controls that can apply to multiple blocks in MultiBlockControls.

Example from the paragraph block:

<MultiBlockControls>
	<MultiBlockAlignmentToolbar
		value={ align }
		onChange={ ( nextAlign ) => {
			setAttributes( { align: nextAlign } );
		} }
	/>
</MultiBlockControls>
<BlockControls>
	... block specific controls would go here ...
</BlockControls>

In this example, the alignment toolbar will be used for single blocks and multiple blocks, there's no need to define it twice. The new MultiBlockControls slot will be used for both, and the alignment toolbar knows if it should be operating on a single block or the list of selected blocks.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Jul 19, 2018

Member

Rebased for the clientId stuff.

Member

notnownikki commented Jul 19, 2018

Rebased for the clientId stuff.

@aduth

I've not taken this for a spin yet, but am curious: Is this expected to work as-is? I'm really liking the new (limited) impact on block API!

return (
<Fragment>
<BlockControls>
<AlignmentToolbar
<MultiBlockControls>

This comment has been minimized.

@aduth

aduth Aug 6, 2018

Member

Does this still support the use-case of the single block selection?

It also prompts me to think: Can we just build-in multi block controls behavior to <BlockControls /> ?

@aduth

aduth Aug 6, 2018

Member

Does this still support the use-case of the single block selection?

It also prompts me to think: Can we just build-in multi block controls behavior to <BlockControls /> ?

This comment has been minimized.

@notnownikki

notnownikki Aug 6, 2018

Member

Yes and no, respectively :)

There needs to be some way to show just the multi block controls when multiple blocks are selected. With this approach, it's two slots, one for multi- and one for single- controls. Both are rendered when only one block is selected, and only the multi slot is rendered when we have multiple blocks selected.

I toyed with having some kind of registry, or attribute on elements, but those approaches didn't seem as clean and as obvious to the block developer.

This PR won't work as-is with the current state of master, but I'm really after feedback on the approach at the moment before I go rebasing.

@notnownikki

notnownikki Aug 6, 2018

Member

Yes and no, respectively :)

There needs to be some way to show just the multi block controls when multiple blocks are selected. With this approach, it's two slots, one for multi- and one for single- controls. Both are rendered when only one block is selected, and only the multi slot is rendered when we have multiple blocks selected.

I toyed with having some kind of registry, or attribute on elements, but those approaches didn't seem as clean and as obvious to the block developer.

This PR won't work as-is with the current state of master, but I'm really after feedback on the approach at the moment before I go rebasing.

This comment has been minimized.

@aduth

aduth Aug 6, 2018

Member

There needs to be some way to show just the multi block controls when multiple blocks are selected. With this approach, it's two slots, one for multi- and one for single- controls. Both are rendered when only one block is selected, and only the multi slot is rendered when we have multiple blocks selected.

So is this just a complexity of the implementation? Or is it conceptually important that, as part of the edit interface, a distinction is made between single- and multi- selection?

@aduth

aduth Aug 6, 2018

Member

There needs to be some way to show just the multi block controls when multiple blocks are selected. With this approach, it's two slots, one for multi- and one for single- controls. Both are rendered when only one block is selected, and only the multi slot is rendered when we have multiple blocks selected.

So is this just a complexity of the implementation? Or is it conceptually important that, as part of the edit interface, a distinction is made between single- and multi- selection?

This comment has been minimized.

@notnownikki

notnownikki Aug 6, 2018

Member

It's a little from column A and a little from column B.

I'd say that allowing the distinction to be made in edit allows block developers to specify if they want controls to apply to multiple blocks or not, however, do they need to? Is a block developer ever going to say, "I have this control that can update multiple blocks at once, and I really don't want the user to do that!"

Probably not.

I'd be very happy to hear ideas on how we could flag up controls that should apply to multiple blocks and therefore sort out the rendering purely with <BlockControls />. All my previous attempts at that smelled wrong.

@notnownikki

notnownikki Aug 6, 2018

Member

It's a little from column A and a little from column B.

I'd say that allowing the distinction to be made in edit allows block developers to specify if they want controls to apply to multiple blocks or not, however, do they need to? Is a block developer ever going to say, "I have this control that can update multiple blocks at once, and I really don't want the user to do that!"

Probably not.

I'd be very happy to hear ideas on how we could flag up controls that should apply to multiple blocks and therefore sort out the rendering purely with <BlockControls />. All my previous attempts at that smelled wrong.

This comment has been minimized.

@gziolo

gziolo Aug 10, 2018

Member

Seeing the implementation:

const MultiBlockAlignmentToolbar = withMultiBlockSupport( AlignmentToolbar, 'align' );

which effectively adds the attributeName for the toolbar control behind the scenes. I would bet that it is possible to express all this code as follows:

<BlockControls>
    <BlockAlignmentToolbar
        name="align"
        multi={ true }
        value={ align }
        onChange={ onChange }
    />
</BlockControls>

It would require some refactoring for BlockControls which is wrapped with ifBlockEditSelected HOC. It would have to support also the case when block is multi selected and have multi set to true. Finally, it would also have to handle the case when multi is set to false and block is multi selected - not render such control at all.

Still, the biggest challenge would be to iterate over those multiplied controls filled into slot and apply some modifications to the virtual dom tree which would allow to group them, render only one but trigger update for all of them. Basically, replicate what withMultiBlockSupport does.

In the future, we could go even further and hide the need for using value and onChange props since they are usually repeatable so the can be derived from the attribute name as explored in #6705.

@gziolo

gziolo Aug 10, 2018

Member

Seeing the implementation:

const MultiBlockAlignmentToolbar = withMultiBlockSupport( AlignmentToolbar, 'align' );

which effectively adds the attributeName for the toolbar control behind the scenes. I would bet that it is possible to express all this code as follows:

<BlockControls>
    <BlockAlignmentToolbar
        name="align"
        multi={ true }
        value={ align }
        onChange={ onChange }
    />
</BlockControls>

It would require some refactoring for BlockControls which is wrapped with ifBlockEditSelected HOC. It would have to support also the case when block is multi selected and have multi set to true. Finally, it would also have to handle the case when multi is set to false and block is multi selected - not render such control at all.

Still, the biggest challenge would be to iterate over those multiplied controls filled into slot and apply some modifications to the virtual dom tree which would allow to group them, render only one but trigger update for all of them. Basically, replicate what withMultiBlockSupport does.

In the future, we could go even further and hide the need for using value and onChange props since they are usually repeatable so the can be derived from the attribute name as explored in #6705.

This comment has been minimized.

@aduth

aduth Aug 10, 2018

Member

In the future, we could go even further and hide the need for using value and onChange props since they are usually repeatable so the can be derived from the attribute name as explored in #6705.

Also #7352

@aduth

aduth Aug 10, 2018

Member

In the future, we could go even further and hide the need for using value and onChange props since they are usually repeatable so the can be derived from the attribute name as explored in #6705.

Also #7352

@gziolo

This is heading in the right direction and is going to be a super useful feature. It is very complex one, but very powerful for all plugin developers so let's make sure that public API is as simple as possible.

It would be lovely if we could explore what @aduth suggested:

Can we just build-in multi-block controls behavior to <BlockControls /> ?

I left my comment about that.

return (
<Fragment>
<BlockControls>
<AlignmentToolbar
<MultiBlockControls>

This comment has been minimized.

@gziolo

gziolo Aug 10, 2018

Member

Seeing the implementation:

const MultiBlockAlignmentToolbar = withMultiBlockSupport( AlignmentToolbar, 'align' );

which effectively adds the attributeName for the toolbar control behind the scenes. I would bet that it is possible to express all this code as follows:

<BlockControls>
    <BlockAlignmentToolbar
        name="align"
        multi={ true }
        value={ align }
        onChange={ onChange }
    />
</BlockControls>

It would require some refactoring for BlockControls which is wrapped with ifBlockEditSelected HOC. It would have to support also the case when block is multi selected and have multi set to true. Finally, it would also have to handle the case when multi is set to false and block is multi selected - not render such control at all.

Still, the biggest challenge would be to iterate over those multiplied controls filled into slot and apply some modifications to the virtual dom tree which would allow to group them, render only one but trigger update for all of them. Basically, replicate what withMultiBlockSupport does.

In the future, we could go even further and hide the need for using value and onChange props since they are usually repeatable so the can be derived from the attribute name as explored in #6705.

@gziolo

gziolo Aug 10, 2018

Member

Seeing the implementation:

const MultiBlockAlignmentToolbar = withMultiBlockSupport( AlignmentToolbar, 'align' );

which effectively adds the attributeName for the toolbar control behind the scenes. I would bet that it is possible to express all this code as follows:

<BlockControls>
    <BlockAlignmentToolbar
        name="align"
        multi={ true }
        value={ align }
        onChange={ onChange }
    />
</BlockControls>

It would require some refactoring for BlockControls which is wrapped with ifBlockEditSelected HOC. It would have to support also the case when block is multi selected and have multi set to true. Finally, it would also have to handle the case when multi is set to false and block is multi selected - not render such control at all.

Still, the biggest challenge would be to iterate over those multiplied controls filled into slot and apply some modifications to the virtual dom tree which would allow to group them, render only one but trigger update for all of them. Basically, replicate what withMultiBlockSupport does.

In the future, we could go even further and hide the need for using value and onChange props since they are usually repeatable so the can be derived from the attribute name as explored in #6705.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 10, 2018

Member

It would be lovely if we could explore what @aduth suggested:

Can we just build-in multi-block controls behavior to ?

I left my comment about that.

FWIW, and along the same lines as mentioned in #7635 (comment), in discussing this with @mtias there's may be an argument to be made that the controls for single selection may not be strictly the same as the controls for multi-selection of a block. I'm not entirely sure what the use-case would be. At the very least then, I think we'd want the two sets of controls to be insulated from one another (i.e. MultiControls doesn't also implement singular controls, even if that means duplicating).

Member

aduth commented Aug 10, 2018

It would be lovely if we could explore what @aduth suggested:

Can we just build-in multi-block controls behavior to ?

I left my comment about that.

FWIW, and along the same lines as mentioned in #7635 (comment), in discussing this with @mtias there's may be an argument to be made that the controls for single selection may not be strictly the same as the controls for multi-selection of a block. I'm not entirely sure what the use-case would be. At the very least then, I think we'd want the two sets of controls to be insulated from one another (i.e. MultiControls doesn't also implement singular controls, even if that means duplicating).

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Aug 10, 2018

Member

At the very least then, I think we'd want the two sets of controls to be insulated from one another (i.e. MultiControls doesn't also implement singular controls, even if that means duplicating).

Does this mean that the withMultiBlockSupport HOC is a no-go, or could we provide that for devs to use with simple controls where it fits nicely? If a developer uses that approach and then in the future finds that it's not going to work, they could replace the component it produced with a custom one without impacting anything else. And for those controls which do purely deal with setting an attribute, it seems a nice time saver, but only as an option to use, not a forced part of the API.

Member

notnownikki commented Aug 10, 2018

At the very least then, I think we'd want the two sets of controls to be insulated from one another (i.e. MultiControls doesn't also implement singular controls, even if that means duplicating).

Does this mean that the withMultiBlockSupport HOC is a no-go, or could we provide that for devs to use with simple controls where it fits nicely? If a developer uses that approach and then in the future finds that it's not going to work, they could replace the component it produced with a custom one without impacting anything else. And for those controls which do purely deal with setting an attribute, it seems a nice time saver, but only as an option to use, not a forced part of the API.

@aduth

Does this mean that the withMultiBlockSupport HOC is a no-go, or could we provide that for devs to use with simple controls where it fits nicely

I'm not entirely clear its purpose, and while I think some "magic" for making attribute assignment easier would be a nice addition (explored also in #6705 and #7352), it's important that it's introduced consistently. If a control assigns attribute values for multi blocks, is there a reason it shouldn't for singular? What controls does this cover? What about support for non-standard attribute names?

I wonder if <MultiBlockControls> could provide context by a render prop function which exposes attributes and setAttributes which read from / apply to all blocks in the multi-selection.

<MultiBlockControls>
	{ ( { attributes, setAttributes } ) => (
		<BlockAlignmentToolbar
			value={ attributes.align }
			onChange={ ( nextAlign ) => {
				setAttributes( { align: nextAlign } );
			} }
		/>
	) }
</MultiBlockControls>

There's a potential issue here in variable shadowing.

*/
export const withMultiBlockSupport = ( component, attributeName ) => createHigherOrderComponent( ( OriginalComponent ) => {
const multSelectComponent = ( props ) => {
const newProps = { ...props };

This comment has been minimized.

@aduth

aduth Aug 10, 2018

Member

This shallow clone may be costly and unnecessary when we don't have a multi selection. I think it should only be done inside the condition.

@aduth

aduth Aug 10, 2018

Member

This shallow clone may be costly and unnecessary when we don't have a multi selection. I think it should only be done inside the condition.

* @return {Component} Component that can handle multple selected blocks.
*/
export const withMultiBlockSupport = ( component, attributeName ) => createHigherOrderComponent( ( OriginalComponent ) => {
const multSelectComponent = ( props ) => {

This comment has been minimized.

@aduth

aduth Aug 10, 2018

Member

Typo: mult -> multi

@aduth

aduth Aug 10, 2018

Member

Typo: mult -> multi

*
* @return {Component} Component which renders only when the BlockEdit is selected or it is the first block in a multi selection.
*/

This comment has been minimized.

@aduth

aduth Aug 10, 2018

Member

This newline shouldn't be here, at risk of the JSDoc not being associated with the function.

@aduth

aduth Aug 10, 2018

Member

This newline shouldn't be here, at risk of the JSDoc not being associated with the function.

* @return {Component} Component which renders only when the BlockEdit is selected or it is the first block in a multi selection.
*/
const isFirstOrOnlyBlockSelectedHOC = createHigherOrderComponent( ( OriginalComponent ) => {

This comment has been minimized.

@aduth

aduth Aug 10, 2018

Member

Re: Naming, I tend to encourage avoiding the "HOC" terminology because it's not always obvious to new developers. We already have a convention around the "with" prefix, that I think we could use here as well.

@aduth

aduth Aug 10, 2018

Member

Re: Naming, I tend to encourage avoiding the "HOC" terminology because it's not always obvious to new developers. We already have a convention around the "with" prefix, that I think we could use here as well.

return ( props ) => {
return (
<Consumer>
{ ( { isSelected, clientId } ) => ( isSelected || ( clientId === props.getFirstMultiSelectedBlockClientId && props.allSelectedBlocksOfSameType ) ) && (

This comment has been minimized.

@aduth

aduth Aug 10, 2018

Member

Line too long.

@aduth

aduth Aug 10, 2018

Member

Line too long.

allSelectedBlocksOfSameType,
};
} ),
] )( isFirstOrOnlyBlockSelectedHOC( component ) );

This comment has been minimized.

@aduth

aduth Aug 10, 2018

Member

This is what compose serves to eliminate, to rewrite:

a( b( c( d( Component ) ) ) );

...as:

compose( [ a, b, c, d ] )( Component );

In other words, isFirstOrOnlyBlockSelectedHOC could be added as an entry of the compose argument array.

@aduth

aduth Aug 10, 2018

Member

This is what compose serves to eliminate, to rewrite:

a( b( c( d( Component ) ) ) );

...as:

compose( [ a, b, c, d ] )( Component );

In other words, isFirstOrOnlyBlockSelectedHOC could be added as an entry of the compose argument array.

*
* @return {*} Reduced value of attribute.
*/
function reduceAttribute( multiSelectedBlocks, attributeName ) {

This comment has been minimized.

@aduth

aduth Aug 10, 2018

Member

For a function called reduceAttribute, why not just Array#reduce ? 馃槃

return multiSelectedBlocks.reduce( ( attribute, block, i ) => {
	const block = multiSelectedBlocks[ i ];
	if ( block.attributes[ attributeName ] === attribute || 0 === i ) {
		return block.attributes[ attributeName ];
	}

	return undefined;
} );
@aduth

aduth Aug 10, 2018

Member

For a function called reduceAttribute, why not just Array#reduce ? 馃槃

return multiSelectedBlocks.reduce( ( attribute, block, i ) => {
	const block = multiSelectedBlocks[ i ];
	if ( block.attributes[ attributeName ] === attribute || 0 === i ) {
		return block.attributes[ attributeName ];
	}

	return undefined;
} );
if ( block.attributes[ attributeName ] === attribute || 0 === i ) {
attribute = block.attributes[ attributeName ];
} else {
attribute = undefined;

This comment has been minimized.

@aduth

aduth Aug 10, 2018

Member

FWIW if the idea is to abort with undefined if we encounter a different attribute, this could be a return; statement, which would help avoid needing to iterate the rest of the array.

(This optimization won't be possible if converted to Array#reduce)

@aduth

aduth Aug 10, 2018

Member

FWIW if the idea is to abort with undefined if we encounter a different attribute, this could be a return; statement, which would help avoid needing to iterate the rest of the array.

(This optimization won't be possible if converted to Array#reduce)

return (
<Fragment>
<BlockControls>
<AlignmentToolbar
<MultiBlockControls>

This comment has been minimized.

@aduth

aduth Aug 10, 2018

Member

In the future, we could go even further and hide the need for using value and onChange props since they are usually repeatable so the can be derived from the attribute name as explored in #6705.

Also #7352

@aduth

aduth Aug 10, 2018

Member

In the future, we could go even further and hide the need for using value and onChange props since they are usually repeatable so the can be derived from the attribute name as explored in #6705.

Also #7352

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Aug 17, 2018

Member

Just to confirm how we want to proceed with this...

  1. Remove the withMultiBlockSupport HOC.
  2. Change <MultiBlockControls> to use a render prop so that the single block toolbar controls get passed the correct reduced attributes, and setAttributes, as shown in #7635 (review)
  3. Rebase on master and apply <MultiBlockControls> on controls that are provided by supports in editor/src/hooks
  4. Address all other review points (typos, code style, etc.)

Does that sound like a plan? Anything that needs to be corrected or added?

Member

notnownikki commented Aug 17, 2018

Just to confirm how we want to proceed with this...

  1. Remove the withMultiBlockSupport HOC.
  2. Change <MultiBlockControls> to use a render prop so that the single block toolbar controls get passed the correct reduced attributes, and setAttributes, as shown in #7635 (review)
  3. Rebase on master and apply <MultiBlockControls> on controls that are provided by supports in editor/src/hooks
  4. Address all other review points (typos, code style, etc.)

Does that sound like a plan? Anything that needs to be corrected or added?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 10, 2018

Member

Rebase on master and apply on controls that are provided by supports in editor/src/hooks

Could you elaborate on what this means? I don't recall where this was discussed.

Otherwise seems reasonable. The point on variable shadowing will probably become painfully obvious, and makes me wonder if there are other avenues to explore (or patterns to establish at least) for addressing. More a practical concern than fundamental issue, though one could argue we should do better establishing that in these examples, attributes and setAttributes should be clarified as to apply to multiple blocks (or not? if it's the "consistent value").

Member

aduth commented Sep 10, 2018

Rebase on master and apply on controls that are provided by supports in editor/src/hooks

Could you elaborate on what this means? I don't recall where this was discussed.

Otherwise seems reasonable. The point on variable shadowing will probably become painfully obvious, and makes me wonder if there are other avenues to explore (or patterns to establish at least) for addressing. More a practical concern than fundamental issue, though one could argue we should do better establishing that in these examples, attributes and setAttributes should be clarified as to apply to multiple blocks (or not? if it's the "consistent value").

@bph

This comment has been minimized.

Show comment
Hide comment
@bph

bph Sep 10, 2018

Contributor

This is a fairly common use case of multi-block formatting in my content creation life:-)
http://recordit.co/ZcXspfFNtK
Would this be possible with this new feature?

Contributor

bph commented Sep 10, 2018

This is a fairly common use case of multi-block formatting in my content creation life:-)
http://recordit.co/ZcXspfFNtK
Would this be possible with this new feature?

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