Opens the block inspector automatically when the block is selected

[youknowriad]

closes #5764

This PR adds the following behavior: It automatically opens the block inspector when a new block is selected.

Testing instructions

• Open the sidebar on the "document" panel
• Select a block
• The block inspector should show up.

[jorgefilipecosta]

I like this helper, can it be part of our data module instead of being part of the edit-post?

[gziolo]

Good call, isn't it the pattern that @aduth is sharing all over again on Slack? 😃

[youknowriad]

Yes, my reluctance to introduce this pattern to the data module comes from it being a generic FP pattern and not something really specific to the data module. I understand the convenience though.

[jorgefilipecosta]

I like this pattern, to execute something after a change in another store. But our effects code is already huge and not easy to organize, this pattern is similar to effects but for other stores so I think it may also become huge when we increase its usage.
Could we have another file with this operations and here we just call a function from that file passing the store to it?

[youknowriad]

I was hesitant here, using another file would mean that these things are not side effects, but they are, just expressed slightly differently, I was hesitant to introduce another "concept".

[gziolo]

It's still better to find it here than in the store file :)

[jasmussen]

Just like how I liked it back in the day when we had the breadcrumbs (Document → [Block Name]) I also like this today. Except for the fact that when you deselect a block, you don't get the document tab back. With the behavior as it is here, it's definitely easier to get to the block tab, but now the Document tab becomes hard to find.

I hate to suggest this, because adding options is almost always a bad idea — but should this be an option, like the "Fix toolbar to top" is an option?

[mtias]

@jasmussen maybe we should just make it so that deselecting a block means selecting the document, so we focus on that tab on block deselect?

[jasmussen]

Yes, that would do it, an I agree that is desired behavior. But it's also essentially the same behavior we had with the breadcrumb setup.

[mtias]

But it's also essentially the same behavior we had with the breadcrumb setup.

There are some notorious differences now in how block selection / deselection works, familiarity with controls, the tabbed interface, etc, that means we should look at it with fresh eyes.

[jasmussen]

I strongly agree. In hindsight my enthusiasm for this feature was perhaps muted by previous feedback, leading me to look for a plan B.

[mtias]

@youknowriad let's switch the tab back to document when un-selecting all blocks. Otherwise looks good to me.

[youknowriad]

I added the switch to document when un-selecting blocks, how does it feel?

[jasmussen]

Here's a GIF of the behavior:

And again, this behavior feels completely obvious to me, and very right. Though, I do also feel like the breadcrumb concept we had when last time we had this behavior, is a better fit.

The problem with the tabs is that tabs usually stay selected. I.e. you pick a tab, and the section shown sticks. If these tabs are suddenly controlled primarily by selecting or deselecting blocks, then ti doesn't really make sense for them to be tabs anymore.

CC: @folletto for thoughts.

[gziolo]

This function could exit early when the sidebar is not opened. This would simplify logic a bit:

if ( ! isSidebarOpened ) {
    return;
}
store.dispatch(
    openGeneralSidebar( `edit-post/${ selectionStart ? 'block' : 'document' }` )
);

[gziolo]

@jasmussen, looking forward to what you come up with. I'm planning to do refactor which aims to increase reuse between those 2 tabs and plugin sidebars. It would be nice to take plugins into account, too. See this for reference:
https://video.twimg.com/tweet_video/DZkH_9tXcAExpWR.mp4

[jasmussen]

looking forward to what you come up with. I'm planning to do refactor which aims to increase reuse between those 2 tabs and plugin sidebars. It would be nice to take plugins into account, too

Just to clarify, this would not affect plugins. Plugins open separate sidebars, and this behavior would only affect the Settings sidebar, which you see when the cog is toggled. If you untoggle the Settings sidebar, or open a different sidebar, no switching of tabs should happen because that sidebar is closed.

By the way here's a blast from the past (version 0.3), which pretty much illustrates what I meant about the breadcrumbs:

[folletto]

And again, this behavior feels completely obvious to me, and very right.

Massive agreement here. 💯

The problem with the tabs is that tabs usually stay selected. I.e. you pick a tab, and the section shown sticks. If these tabs are suddenly controlled primarily by selecting or deselecting blocks, then ti doesn't really make sense for them to be tabs anymore.

This is also a good consideration. Tabs in general aren't meant to stick around. Then again, also breadcrumbs aren't really meant to switch automatically. ;)

I went back to existing apps to check. Here's Keynote:

Here's Sketch:

In short, the expected behaviour seems to be... removing that top "Document" / "Block" thing, either tabs or breadcrumb, and let the content vary depending on what's selected: if nothing is selected, the Document shows. If something is selected, the Block gets selected.

I'd note that this wasn't something that was on the table at the beginning of the project because we didn't have settled plugins. As now they open in their own sidebar, and not in a new sidebar tab, I think we can do this.

tl;dr: let's remove "tabs" entirely.

[mtias]

Not sure about removing the tabs entirely and whether that makes it harder to discover "Document". I think we should merge this as is and open another PR to explore removing tabs — we might still need a title/role at the top.

[folletto]

Yep. I just wanted to show that it's an established standard to not have any selector at the top. Even if we might end up keeping something there, I think it's important to be aware that's something we are introducing as an extra.

This is definitely something to be tested tho, and I agree to merge this first.

[gziolo]

I'd note that this wasn't something that was on the table at the beginning of the project because we didn't have settled plugins. As now they open in their own sidebar, and not in a new sidebar tab, I think we can do this.

This needs to be tested together with the existing plugin because it might be confusing. I can imagine the scenario where plugin also wants to replicate the same behavior that is being discussed for the document & block pair.

[folletto]

This needs to be tested together with the existing plugin because it might be confusing. I can imagine the scenario where plugin also wants to replicate the same behavior that is being discussed for the document & block pair.

I agree on testing — but I'm not sure what you mean with this. If they want to replicate the same behaviour they will do it, regardless of tabs or not, no? What would be confusing exactly?

[gziolo]

I agree on testing — but I'm not sure what you mean with this. If they want to replicate the same behaviour they will do it, regardless of tabs or not, no? What would be confusing exactly?

How to switch between plugin and document, might be not intuitive.

[folletto]

Ok, got it. :)
Yes, definitely has to work very well there too! :)

[aduth]

The new $( document ).ready ? This very much concerns me, as it is not at all scalable.

We seem to be contradicting ourselves that stores ought be independent from one another. What we're doing here is effectively updating a store in response to actions dispatched in other stores.

It would be as if the sidebar reducer were to include core/editor/SELECT_BLOCK as a switch case. This is one option. Obviously means exposing internals of editor store to other modules and its various related actions.

Another option is some API for forwarding specific action dispatchers from other stores (forward( 'core/editor ).selectBlock( /* ... ? */ )), though I think this suffers similarly, and either means exposing same internals, or imposing on listener to handle arguments independently.

While I don't think it applies here because it is an effect, not really a composed value, for many cases where we'd be inclined to add here (and it will be oh so convenient to do so), a better option would be a composing selector where a module's own selectors also select from other modules. We don't have any instances of this so far, and calling select within a selector feels somehow wrong, though the general idea of it seems valid.

Ultimately I think it should be something like what's done here with subscribing to selector values, though ideally in a more declarative fashion. Roughly I see three critical pieces of information:

• The selector(s) returning the value
• The condition on which a respondent action should dispatch
  • Minimally, just the new value itself
• The action to dispatch

Further, I don't even know that this needs to be a Redux-specific concept. Really it's a general pattern for responding to changes in values. I hesitate to mention it, but this is something decently covered in the proposed Observable specification.

Scribbling a bit:

const { hasSelectedBlock } = select( 'core/editor' );
const { isViewportMatch } = select( 'core/viewport' );

new DispatchObservable( subscribe, [
	[ hasSelectedBlock, true, () => setActiveSidebar( 'edit-post/block' ) ],
	[ hasSelectedBlock, false, () => setActiveSidebar( 'edit-post/document' ) ],
	[ () => isViewportMatch( '< medium' ), true, closeGeneralSidebar ],
] ).subscribe( ( action ) => store.dispatch( action ) );

Where subscribe could be any function for starting to listen, returning an observable whose callback is invoked only when actions are to be dispatched. I think the selectionStart && isSidebarOpened logic could be simplified to handle the active sidebar regardless of whether or not it is currently open.

This comment is very much a brain-dump of unfinished thoughts / suggestions, but I want this to be much more unintuitive than it is as proposed, as otherwise developers will add to this INIT effect carelessly.

Currently this gives me vibes of jQuery soup: http://tutorials.jenkov.com/jquery/critique.html#jquery-big-main-method

[youknowriad]

Good thoughts and I agree with the general feeling that this INIT action is not great. And yes, the implementation here is just an implementation of returning Observables from other Observables (the original subscribe).

You'd need to create this observable somewhere though, and that's the INIT action here. Previously, it was run directly after the store creation in store/index, this PR adds the INIT function as a synthetic change to gather it with the effects. But the same could be done by just moving this to a separate file and call it from the store/index.

Now, whether to introduce explicit Observables instead of custom onChangeListener which is basically the exact same thing written differently, I'm open to change here.

[gziolo]

We seem to be contradicting ourselves that stores ought be independent from one another. What we're doing here is effectively updating a store in response to actions dispatched in other stores.

Yes, this is the biggest concern I see with this approach. On the other hand, if you look at how modules depend on each other, it makes sense to be able to provide this kind of communication between stores. edit-post depends on both editor and viewport so we don't violate the overall architecture of the application.

[aduth]

I don't disagree that we need to initialize some subscribing behavior. My issue is that there is no pattern for organization aligning to what it is we're expressing with these subscribe callback. So instead various behaviors will be added here, whether or not appropriate to do so, and the effect handler will grow into an unmaintainable mess.

[gziolo]

I had one comment: #5882 (comment).

Otherwise, it looks like a nice improvement in comparison what we had before. In particular, it moves the inline logic executed just after the store gets created.