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

Update: Use single block editor in the widgets screen #22140

Merged
merged 1 commit into from
May 18, 2020

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented May 6, 2020

Description

All the other screens, we have used a single block editor. The widget screen used multiple block editors. The fact multiple block editors were used always presented additional complexity, e.g., we needed a system external to the block editor that tracks which widget area is active, and then we needed to use slots with bubblesvirtually to show UI pieces like the inserter bock inspector for the currently selected widget area.
On each feature we want to include, we would need to have some kind of custom implementation, e.g.: adding a block navigator for all the widget areas would evolve some complex slot fills to have it working.
I was trying to add the block navigator and tried to move them to a single block editor instead. The move proved to be very simple; we already have all the pieces to save the inner blocks of a block in a custom location, and it worked well. I ended up spending most of the time of this PR, fixing styling issues and handling things like events to unselect blocks, etc.
But things seem to be working well, and this PR ends up adding a new feature to the widget screen while removing 100+ lines of code.

The change also as some a11y advantages e.g., now we can use block editor select mode (ESC keypress) to navigate around widget areas, and tabbing is similar to other screens while previously tabbing was inconsistent and different from the other screens.

I think applying this change makes the widget screen even simpler and will make the future maintenance of the screen much easier is it now works like the other screens.

How has this been tested?

I verified the widget screen still worked well, allowed saving, etc.
I verified I could use the block navigator on the widget screen.

@jorgefilipecosta jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Type] Code Quality Issues or PRs that relate to code quality [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets labels May 6, 2020
@github-actions
Copy link

github-actions bot commented May 6, 2020

Size Change: -255 B (0%)

Total Size: 835 kB

Filename Size Change
build/block-editor/index.js 105 kB +9 B (0%)
build/block-library/editor-rtl.css 7.22 kB +31 B (0%)
build/block-library/editor.css 7.22 kB +31 B (0%)
build/block-library/index.js 118 kB +219 B (0%)
build/edit-widgets/index.js 7.37 kB -492 B (6%)
build/edit-widgets/style-rtl.css 4.67 kB -28 B (0%)
build/edit-widgets/style.css 4.67 kB -25 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.93 kB 0 B
build/block-directory/style-rtl.css 790 B 0 B
build/block-directory/style.css 791 B 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 182 kB 0 B
build/components/style-rtl.css 17 kB 0 B
build/components/style.css 17 kB 0 B
build/compose/index.js 6.67 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/index.js 6.6 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/index.js 28.1 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 12 kB 0 B
build/edit-site/style-rtl.css 5.22 kB 0 B
build/edit-site/style.css 5.22 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.3 kB 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.64 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

} );
} )
);
}, [ areas ] );
Copy link
Member

Choose a reason for hiding this comment

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

Should blocks also be listed in the dependency array?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch @ZebulanStanphill, I think we should list it 👍

@jorgefilipecosta jorgefilipecosta force-pushed the try/use-single-block-editor-for-widgets branch 2 times, most recently from 0f9b2a2 to c4ed88a Compare May 11, 2020 17:42
@jorgefilipecosta jorgefilipecosta force-pushed the try/use-single-block-editor-for-widgets branch 2 times, most recently from 48da84a to 0ca1b33 Compare May 12, 2020 17:05
@jorgefilipecosta jorgefilipecosta force-pushed the try/use-single-block-editor-for-widgets branch from 0ca1b33 to a1c15cb Compare May 18, 2020 20:41
@jorgefilipecosta jorgefilipecosta force-pushed the try/use-single-block-editor-for-widgets branch from a1c15cb to 8746f6d Compare May 18, 2020 21:12
@jorgefilipecosta
Copy link
Member Author

This PR is a dependency for some work we are doing on the widgets screen. I'm going to merge the PR so the work can advance more easily. In case there is any comment regarding a change done here please leave a comment and I will gladly address it as a follow-up PR.

Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

Hm, nothing stood out to me as problematic after seeing #22447.

);
return (
<InnerBlocks
__experimentalBlocks={ blocks }
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 changed to value now that #21368 has been merged

@mcsf mcsf deleted the try/use-single-block-editor-for-widgets branch May 19, 2020 09:49
noahtallen added a commit that referenced this pull request May 19, 2020
In #22140, a change was made to the inserter to insert the block at the current selection. Since the e2e test had selected test inside a nested template part, the block was being inserted there, though it was supposed to be inserted at the end of the document. Before that PR, the behavior was to insert it at the end of the document, and not at the current selection.

The fix is to select the document in the block breadcrumb before adding the block. This clears any selection so that adding the block block dirties the root entity and not a nested entity.
} = select( 'core/block-editor' );
const { getBlockVariations } = select( 'core/blocks' );

rootClientId =
rootClientId || getBlockRootClientId( clientId ) || undefined;
rootClientId ||
getBlockRootClientId( clientId || getBlockSelectionEnd() ) ||
Copy link
Contributor

@talldan talldan Jul 3, 2020

Choose a reason for hiding this comment

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

#23263 proposes to revert this particular change, as it caused the default root appender to insert blocks wherever selection is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems logical @talldan the widget screen should inject the rootClientId as a prop of the inserter instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants