Try multi-select inspector for same blocks #3535

ellatrix · 2017-11-17T16:37:06Z

Description

WIP, only works for the paragraph block.

This is an attempt to add inspector controls when there are multiple blocks selected of the same type. This requires us to move away the inspector rendering to a separate component so that it can be reused ~~(PR does not include change for the edit component yet)~~. I realise some quite like that everything is rendered in one big edit component, but I don't see how we could achieve this without splitting it up. @aduth @youknowriad @mtias What do you think about this?

This only adds the controls to the inspector. The same should then be done for the toolbar on the top.

ellatrix · 2017-11-17T16:39:16Z

editor/components/block-inspector/index.js

+
+			const attributeArray = multiSelectedBlocks.map( ( block ) => block.attributes );
+			const attributeKeys = uniq( flatten( attributeArray.map( keys ) ) );
+			const attributes = attributeKeys.reduce( ( acc, key ) => {


Attempted to use lodash's intersection or similar, but not quite sure how to get it to work.

codecov · 2017-11-17T16:42:35Z

Codecov Report

Merging #3535 into master will increase coverage by 0.06%.
The diff coverage is 3.33%.

@@            Coverage Diff             @@
##           master    #3535      +/-   ##
==========================================
+ Coverage    34.9%   34.97%   +0.06%     
==========================================
  Files         263      263              
  Lines        6727     6771      +44     
  Branches     1227     1235       +8     
==========================================
+ Hits         2348     2368      +20     
- Misses       3694     3714      +20     
- Partials      685      689       +4

Impacted Files	Coverage Δ
blocks/library/paragraph/index.js	`26.92% <0%> (-2.25%)`	⬇️
editor/components/block-inspector/index.js	`3.57% <4.34%> (-10.72%)`	⬇️
editor/store.js	`81.81% <0%> (-1.52%)`	⬇️
blocks/api/registration.js	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b87f59c...8b6d3ec. Read the comment docs.

ellatrix · 2017-12-11T13:33:55Z

Any thoughts on this at all? :)

youknowriad · 2017-12-11T13:38:50Z

blocks/library/paragraph/index.js

@@ -93,9 +92,55 @@ registerBlockType( 'core/paragraph', {
 		}
 	},

+	inspector( { attributes, setAttributes } ) {


A little bit concerned about this. Some times we need local state to be shared beween the inspector, toolbar and the edit. If we separate this, it's not possible anymore.

Do you think it's possible to add some magic to InspectorControls to detect if it's a multiselection and only render for one block only or something?

I tried it before, but can't remember it well, so I'll try it again. Could you give an example of local state, just for me to think about solutions for it? :)

We don't have an exemple with shared local state between the edit and BlockInspector but we do have between edit and BlockControls (in the audio or block blocks) and I believe these two are similar because in the exact same way we can display the same block toolbar in we multi-select similar blocks.

Exactly, what is done here for the inspector should then also be done for the toolbar. The problem with the edit function is that it makes use for focus. We'd need to remove that and figure out how to do that differently. I'll find the example with shared state and think about it.

@youknowriad Which block are you referring to with the local state? Is there anything that wouldn't be solvable with block attributes? Ideally, we should stick with those for state.

@iseulde block attributes are "state" but they are "state" persisted in post_content or comments or meta. I don't know if it's a good idea to use them for "local state", we may end up serializing useless local state attributes in comments.

For example, the audio block triggers an editing local state using a button toolbar. And this local state changes the UI of the block.

I'm looking at this for the block toolbar (so if the selected blocks are of the same type, they get the toolbar and the controls apply changes to all selected blocks.)

It seems to me that having an inspector and toolbar function like this is very close to what we need, but as we need to share state between edit and BlockInspector/Toolbar, we need to reuse those functions to fill the appropriate slots.

The render in edit would look like this:

<BlockControls> { this.toolbar( { state, attributes, setAttributes } ) } </BlockControls> <InspectorControls> { this.inspector( { state, attributes, setAttributes } ) } </InspectorControls>

The slots would get filled as normal, single block selection behaviour in unchanged.

Then, for example, in the block inspector where we currently have "Coming Soon", we'd get the block settings for the type of blocks we have selected, and call settings.inspector passing an appropriate state, attributes, and a setAttributes function that would set attributes across all selected blocks. The state and attributes could be reduced from the selected blocks, so that if all blocks had the same value for a control, it would get that value (for example, if all paragraphs were the same color, that color would be selected).

aduth · 2017-12-11T14:05:35Z

I think this does identify an issue with how we're treating edit rendering as a single component tree, since this only really works when inspector rendering is one-to-one, and we don't need to externally isolate just the inspector or toolbar behaviors. While that can work well for operating on single blocks, it doesn't work so well for sharing rendering controls across multiple selected blocks.

There's a few reasons I like keeping the slotted elements within edit is that:

It keeps the block API nicely segmented between "edit", "save", and the rest "meta". Splitting out a separate inspector property starts to muddy the distinction, since it's still properly relevant to the editing behaviors.
It forces the block implementer to be aware of the state of their block in the rendering behavior in deciding to render controls only when focused
- This could be argued as a downside
It allows render logic / assigned variables scope to be shared between the form, inspector, and toolbar elements

It's quite late to be making these breaking changes to the API, but if we were, I'd suggest at least:

Be consistent across the board, not just isolating inspector controls, but also toolbar
Maybe doing so in a way that still preserves relationship between editing-relevant rendering, e.g. maybe defining edit as an object of form, inspector, toolbar? Optionally so?

If we were to build support for these to be separated, would we then handle the focus conditional rendering on behalf of the block implementer?

Noting that I've had similar issues with edit's slot / fill behavior in interoperability attempts.

ellatrix · 2017-12-13T13:40:22Z

#3483 also adds an interesting case where the controls would need the node of the block...

ellatrix · 2017-12-13T13:58:28Z

Trying the other approach again.

ellatrix · 2017-12-15T10:43:20Z

If we want to keep all components in one edit function, it seems we'll have to do something like this to change the attributes and setAttributes function for more than one block. Based on the id, InspectorControls can also decide when to display something, the block implementor no longer needs to worry about focus there. @aduth @youknowriad How does this sound? I'm not sure what alternatives we have...

return [
	<InspectorControls key="inspector" id={ id }>
		{ ( { attributes, setAttributes } ) => [
			<BlockDescription>
				<p>{ __( 'Text. Great things start here.' ) }</p>
			</BlockDescription>,
		] }
	</InspectorControls>,
];

aduth · 2017-12-18T14:13:49Z

Would it be too much magic for the ID to be "injected" via React context so the block implementer doesn't need to concern themselves with it? Thinking each BlockEdit could provide its block ID as context which InspectorControls reads from.

The function as child makes sense to me, but I think could be disorienting for block implementers. Also suffers from variable shadowing.

gziolo · 2018-08-10T05:49:56Z

Should we also close this one in favor of #7635?

catehstn · 2018-11-06T17:38:16Z

Closing in favour of #7635.

ellatrix requested review from youknowriad, mtias and aduth November 17, 2017 16:37

ellatrix commented Nov 17, 2017

View reviewed changes

WIP

8b6d3ec

ellatrix force-pushed the try/multi-inspector branch from 8271c93 to 8b6d3ec Compare November 17, 2017 16:48

youknowriad reviewed Dec 11, 2017

View reviewed changes

aduth mentioned this pull request Dec 15, 2017

Block List: Move deselect behavior to block component #3743

Closed

mcsf mentioned this pull request Dec 21, 2017

Multi-select unconnected blocks #2320

Closed

notnownikki mentioned this pull request Jun 29, 2018

Try a toolbar that affects multiple selected blocks #7635

Closed

ZebulanStanphill mentioned this pull request Aug 10, 2018

Add multi select inspector #1811

Closed

ocean90 mentioned this pull request Nov 3, 2018

Add multi-selection toolbar and inspector controls support #11446

Closed

catehstn closed this Nov 6, 2018

youknowriad deleted the try/multi-inspector branch November 6, 2018 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try multi-select inspector for same blocks #3535

Try multi-select inspector for same blocks #3535

ellatrix commented Nov 17, 2017 •

edited

Loading

ellatrix Nov 17, 2017

codecov bot commented Nov 17, 2017 •

edited

Loading

ellatrix commented Dec 11, 2017

youknowriad Dec 11, 2017

ellatrix Dec 11, 2017

youknowriad Dec 11, 2017

ellatrix Dec 11, 2017

ellatrix Dec 11, 2017

youknowriad Dec 11, 2017

youknowriad Dec 11, 2017

notnownikki Jun 28, 2018

aduth commented Dec 11, 2017 •

edited

Loading

ellatrix commented Dec 13, 2017

ellatrix commented Dec 13, 2017

ellatrix commented Dec 15, 2017

aduth commented Dec 18, 2017

gziolo commented Aug 10, 2018

catehstn commented Nov 6, 2018

Try multi-select inspector for same blocks #3535

Try multi-select inspector for same blocks #3535

Conversation

ellatrix commented Nov 17, 2017 • edited Loading

Description

Choose a reason for hiding this comment

codecov bot commented Nov 17, 2017 • edited Loading

Codecov Report

ellatrix commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Dec 11, 2017 • edited Loading

ellatrix commented Dec 13, 2017

ellatrix commented Dec 13, 2017

ellatrix commented Dec 15, 2017

aduth commented Dec 18, 2017

gziolo commented Aug 10, 2018

catehstn commented Nov 6, 2018

ellatrix commented Nov 17, 2017 •

edited

Loading

codecov bot commented Nov 17, 2017 •

edited

Loading

aduth commented Dec 11, 2017 •

edited

Loading