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

Lighter Block: allow block types to render their own wrapper #19701

Merged
merged 2 commits into from
Feb 26, 2020

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jan 16, 2020

Description

This is still a work in progress as there are some final details to be figured out.

This is the final step in lightening the block DOM. This PR allows block types to opt in to rendering their own wrapper.

supports: {
   lightBlockWrapper: true,
}

Why?

  • If a block type opts in, there is no longer a need for the getEditWrapperProps setting. Blocks can now apply the props themselves. The could be useful for e.g. alignment. A block can now set proper classes instead of data-align attributes, which will make it easier for themes to style the editor. Currently, for many themes, even the latest core theme, alignment in the editor doesn't reflect the alignment on the front-end.
  • As we lighten the InnerBlocks DOM, it will be possible to build more complex block that need a semantic relationship with its child blocks. E.g. for a table and list block, the table cells and list items need to be direct children of the parent block, so these blocks need to render their own wrapper.
  • This also means that complex nested blocks become much easier to style by themes as the element tree in the editor matches that of the front-end.
<div class="wp-block-columns">
  <div class="wp-block-column">
    <p>one</p>
  </div>
  <div class="wp-block-column">
    <p>two</p>
  </div>
</div>

The above is much nicer to handle than many layers of nested div elements.

I believe that allowing simple block markup in the editor will be essential for full site editing and theming.

  • For blocks that opt in, it will get rid of the useless focusable wrapper element. For example, a paragraph block has currently two focusable elements: the wrapper and the paragraph. The focusable wrapper is useless here, and adds an extra tab stop. There's no reason for the paragraph not to be the wrapper.

Screenshot 2020-01-16 at 13 52 17

Implementation notes

The way I'm approaching this is a bit inspired by react-spring. I created a base block component that is provided with the context it needs to render and the props that the block Edit component may pass. For every element that can act as a block wrapper, I created a wrapped component as properties of the base component, so you can have Block.p, Block.div, Block.figure etc.

This works well together with RichText too. You can pass the component through the tagName props, and RichText will render the wrapper.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

);
} );

const elements = [ 'p', 'div' ];
Copy link
Member Author

@ellatrix ellatrix Jan 16, 2020

Choose a reason for hiding this comment

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

To do: add list of all block level elements.

Copy link
Member

Choose a reason for hiding this comment

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

@chrisvanpatten
Copy link
Member

I was just wondering how this might be approached and this is super cool to see!

@chrisvanpatten
Copy link
Member

I wonder if there could be overlap between these new block wrapper elements and the @wordpress/primitives package that was introduced?

@ellatrix
Copy link
Member Author

@chrisvanpatten Could you elaborate?

@@ -18,6 +18,10 @@ describe( 'block mover', () => {
// Select a block so the block mover is rendered.
await page.focus( '.block-editor-block-list__block' );

// Move the mouse to show the block toolbar
await page.mouse.move( 0, 0 );
await page.mouse.move( 10, 10 );
Copy link
Member Author

Choose a reason for hiding this comment

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

This is necessary because the typing observer does not set isTyping to false when a text field receives focus. Perhaps this can be adjusted in a follow-up PR.

@ellatrix ellatrix force-pushed the try/block-wrapper branch 7 times, most recently from db4f5ed to 1600346 Compare January 25, 2020 19:46
@ellatrix ellatrix added the [Feature] Block API API that allows to express the block paradigm. label Jan 25, 2020
@ellatrix ellatrix marked this pull request as ready for review January 25, 2020 20:40
await editorPage.keyboard.type( 'content 2' );
await editorPage.keyboard.press( 'End' );
await editorPage.keyboard.press( 'Backspace' );
await editorPage.keyboard.type( '2' );
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 changed the above test because it was failing specifically on Travis and not when testing manually or running the e2e tests locally. I throttled CPU, but the tests still passed locally. I've looked into this issue for over a day, and couldn't find what's causing the problem.

I did find a reduced test case that fails:

it( 'test', async () => {
  await page.keyboard.type( 'title 1' );
  await page.keyboard.press( 'Tab' );
  await page.keyboard.type( 'content 1' );
  await page.click( '.editor-post-title__input' );
  await pressKeyWithModifier( 'primary', 'a' ); // <== Remove this line and the test passes...
  await page.keyboard.type( 'title 2' );
  await page.keyboard.press( 'Tab' );
  await pressKeyWithModifier( 'primary', 'a' );
  await page.keyboard.type( 'content 2' );

  expect( await getEditedPostContent() ).toMatchSnapshot();
} );

So it looks like it has something to do with Cmd+A in the title, before doing the same in a paragraph. It's worth mentioning that we are emulating Cmd+A, so maybe it has to do with that and a change to the paragraph block.

@youknowriad youknowriad requested review from a team and mtias January 27, 2020 09:54
@mapk
Copy link
Contributor

mapk commented Feb 17, 2020

Just pinging some theme folks for more eyes, @kjellr and @allancole.

@allancole
Copy link

WOW. This PR sounds excellent! Especially, if the purpose is to make writing CSS rules for the frontend and the backend identical. I agree that having this initially be an opt-in solution will give themers time to adapt to the change and get things right before it becomes the default. Super exciting :-)

@github-actions
Copy link

github-actions bot commented Feb 25, 2020

Size Change: -189 B (0%)

Total Size: 865 kB

Filename Size Change
build/block-editor/index.js 104 kB +380 B (0%)
build/block-editor/style-rtl.css 10.3 kB -30 B (0%)
build/block-editor/style.css 10.3 kB -31 B (0%)
build/block-library/editor-rtl.css 7.67 kB -3 B (0%)
build/block-library/editor.css 7.67 kB -2 B (0%)
build/block-library/index.js 116 kB +15 B (0%)
build/edit-post/index.js 90.9 kB +8 B (0%)
build/edit-post/style-rtl.css 8.59 kB -93 B (1%)
build/edit-post/style.css 8.58 kB -89 B (1%)
build/edit-widgets/index.js 4.43 kB +70 B (1%)
build/edit-widgets/style-rtl.css 2.59 kB -207 B (7%)
build/edit-widgets/style.css 2.58 kB -207 B (8%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-library/style-rtl.css 7.49 kB 0 B
build/block-library/style.css 7.49 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.6 kB 0 B
build/components/index.js 190 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.5 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-site/index.js 4.59 kB 0 B
build/edit-site/style-rtl.css 2.77 kB 0 B
build/edit-site/style.css 2.76 kB 0 B
build/editor/editor-styles-rtl.css 325 B 0 B
build/editor/editor-styles.css 327 B 0 B
build/editor/index.js 44.6 kB 0 B
build/editor/style-rtl.css 4.01 kB 0 B
build/editor/style.css 4 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.6 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 1.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.45 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 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 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.02 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.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 879 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@ellatrix
Copy link
Member Author

Thanks for the positive feedback! Let's merge, iterate, and continue work in #19910.

@ellatrix ellatrix merged commit 67e2df7 into master Feb 26, 2020
@github-actions github-actions bot added this to the Gutenberg 7.7 milestone Feb 26, 2020
@youknowriad
Copy link
Contributor

Great work here. Looking forward to see how this translates to all blocks (and potentially float refactoring)

@jasmussen
Copy link
Contributor

👏

ᕕ( ᐛ )ᕗ

jasmussen pushed a commit that referenced this pull request Feb 26, 2020
The behavior of manipulating blocks in the recent G2 refresh regressed slightly in #19701, this PR addresses that.

Specifically, the blue border is meant to indicate _block focus_, i.e. when it is present you can press Delete to remove the block. It is not meant to indicate the block boundaries when focus is inside text.
jasmussen added a commit that referenced this pull request Feb 26, 2020
The behavior of manipulating blocks in the recent G2 refresh regressed slightly in #19701, this PR addresses that.

Specifically, the blue border is meant to indicate _block focus_, i.e. when it is present you can press Delete to remove the block. It is not meant to indicate the block boundaries when focus is inside text.
@youknowriad youknowriad added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Feb 27, 2020
@youknowriad youknowriad deleted the try/block-wrapper branch February 27, 2020 09:31
@youknowriad
Copy link
Contributor

Added the "Needs Dev Note" label. I believe we need to document this change (update in the different blocks DDOM structure) for devs even if we don't make the API stable yet.

@ellatrix
Copy link
Member Author

This experimental API is still being revised a bit, so it seems premature for a Dev Note.

@ellatrix ellatrix removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants