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 block inspector to the widget screen #16203

Merged
merged 1 commit into from
Jun 25, 2019

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Jun 17, 2019

Description

This PR adds block inspector support to the widget screen.

This PR uses a SelectionObserver component extracted from #14715 by @aduth that ensures that if a block is selected in a given block area, blocks from other areas become unselected.

I tried to implement that SelectionObserver as a hook, in fact, I spent a considerable amount of time with that, and all the possible hook implementations I tried (including one that is equal to this one but uses a usePrevious hook) failed. I'm not sure why but it seems, in this case, useEffect does behave the same way as a componentDidUpdate cc: @aduth as you may have some insights.

How has this been tested?

I selected a theme with multiple block areas (Twenty Fourteen).
I added blocks in the widget screen and verified the block inspector appeared correctly. I verified that if I switch between block areas the inspector behavior is still correct (shows the last selected block independently of the area).

@jorgefilipecosta jorgefilipecosta force-pushed the add/block-inspector-to-the-widget-screen branch from 328bd21 to dfa77b2 Compare June 17, 2019 19:14
}

export default compose( [
withSelect( ( select ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

useSelect, useDispatch? ;)

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 tried to use hooks for this component but hooks did not work as expected, I always got unexpected results.
I tried to use a simple version where I relied on effects that execute when prop changes, and I also tried a usePrevious hook https://usehooks.com/usePrevious/ that allowed an implementation almost equal to this class component version, and both versions did not work properly. It is a very strange case I may be missing something.

@@ -10,8 +11,18 @@ import { withSelect } from '@wordpress/data';
import WidgetArea from '../widget-area';

function WidgetAreas( { areas, blockEditorSettings } ) {
const [ selectedArea, setSelectedArea ] = useState( 0 );
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really need this here? It feels like this could be local state for WidgetArea

Copy link
Member Author

Choose a reason for hiding this comment

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

The widget area controls which of the areas contains a selected block, when a new area is selected other becomes unselected, each WidgetArea is a sibling of each other and siblings can not "communicate" between each other. This local state in WidgetAreas that is the parent of each WidgetArea allows the WidgetArea components to communicate with each other, when one becomes selected the others become unselected and the corresponding effects get executed, I don't think is possible to make this state part of the WidgetArea component.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok makes sense.

@gziolo gziolo added [Package] Block editor /packages/block-editor [Package] Edit Widgets /packages/edit-widgets [Type] Task Issues or PRs that have been broken down into an individual action to take labels Jun 20, 2019
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

It feels like a good start. Two things I think need to be improved:

  • Clicking outside should deselect everything
  • The "no block selected" message was not showing even if I had no block selected (clicking the panel header of the widget area)

@jorgefilipecosta jorgefilipecosta force-pushed the add/block-inspector-to-the-widget-screen branch from dfa77b2 to 1363fc8 Compare June 25, 2019 12:53
@jorgefilipecosta jorgefilipecosta force-pushed the add/block-inspector-to-the-widget-screen branch from 1363fc8 to 3dc985e Compare June 25, 2019 13:23
@jorgefilipecosta jorgefilipecosta merged commit 64a8a7c into master Jun 25, 2019
@jorgefilipecosta jorgefilipecosta deleted the add/block-inspector-to-the-widget-screen branch June 25, 2019 14:32
@youknowriad youknowriad added this to the Gutenberg 6.1 milestone Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Package] Edit Widgets /packages/edit-widgets [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants