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

Try: Support block `edit` defined as tag name for web components interoperability #2791

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@aduth
Member

aduth commented Sep 25, 2017

Related: #2463

This pull request seeks to add support for defining a block's edit property as a tag name, in an effort to explore support for block editable UIs defined as custom elements. The idea being that custom elements may serve as a baseline interface by which the editor creates a block element to be shown in the Gutenberg application. While granting that custom elements are in many ways not an ideal pattern for authoring blocks, they may work well enough as the low-level platform primitive for WordPress to target, and framework wrappers may serve to fill the gap of developer usability.

As an example, consider the following plugin which implements a Gutenberg "Stars" block using Vue, with VueCustomElement providing the necessary wrapper to expose the Vue component as a custom element:

https://gist.github.com/aduth/369767f8153eaad1d955a8022f14ec34

Similar wrappers exist for other frameworks, and in fact some frameworks like Polymer implement their components as custom elements, and yet others like Stencil compile at build time to custom elements.

Implementation notes:

When assigning props to a custom element in DOM reconciliation, React currently does not do well to distinguish between attributes and properties. This was originally slated for improvements in React 16, and while treatment of unknown attributes was improved, this particular failed distinction still exists and was noted as "Future Work" now slated for React 17.

To work around this, a new ComponentInterop component is included to bypass default React reconciliation for custom elements. Currently, it assigns block attributes as DOM attributes via node.setAttribute, and all other block props as properties of the node. This implementation is open for discussion, and may need to be improved and/or alternative terminology may need to be used to clarify between block attributes and DOM attributes, should the distinction need to be made.

Testing instructions:

Download and install the reference Vue custom element block as a plugin to a WordPress installation:

  1. Click "Download ZIP" in the top-right of the Gist page
  2. On a WordPress site running Gutenberg, navigate to Plugins > Add New in the dashboard
  3. Click "Upload Plugin"
  4. Upload, install, and activate the ZIP file downloaded in Step 1
  5. Navigate to Gutenberg > New Post
  6. Using the Inserter, insert a new Stars block
  7. Note that the editable preview is shown and can be interacted by clicking a star to change
@BE-Webdesign

This comment has been minimized.

Show comment
Hide comment
@BE-Webdesign

BE-Webdesign Sep 25, 2017

Contributor

Amazing work aduth!

I verified that this works with regular web components, ones created by Polymer, and ones created by Stencil as well. This is really cool stuff. Very excited about this, I think others will be as well. I have a commit that fixes the test if you would like me to push.

Contributor

BE-Webdesign commented Sep 25, 2017

Amazing work aduth!

I verified that this works with regular web components, ones created by Polymer, and ones created by Stencil as well. This is really cool stuff. Very excited about this, I think others will be as well. I have a commit that fixes the test if you would like me to push.

Fix test.
Updates test to reflect new valid block type registration.
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 25, 2017

Member

I have a commit that fixes the test if you would like me to push.

Feel free!

Member

aduth commented Sep 25, 2017

I have a commit that fixes the test if you would like me to push.

Feel free!

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 25, 2017

Codecov Report

Merging #2791 into master will increase coverage by 0.61%.
The diff coverage is 2.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2791      +/-   ##
==========================================
+ Coverage   33.82%   34.44%   +0.61%     
==========================================
  Files         189      192       +3     
  Lines        5661     6161     +500     
  Branches      988     1124     +136     
==========================================
+ Hits         1915     2122     +207     
- Misses       3171     3396     +225     
- Partials      575      643      +68
Impacted Files Coverage Δ
editor/component-interop/index.js 0% <0%> (ø)
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
blocks/api/registration.js 100% <100%> (ø) ⬆️
editor/sidebar/post-schedule/index.js 63.15% <0%> (-3.51%) ⬇️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/index.js 0% <0%> (ø) ⬆️
editor/inserter/index.js 0% <0%> (ø) ⬆️
editor/sidebar/table-of-contents/index.js 0% <0%> (ø)
editor/sidebar/table-of-contents/item.js 0% <0%> (ø)
... and 7 more

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 95c9713...f32d649. Read the comment docs.

codecov bot commented Sep 25, 2017

Codecov Report

Merging #2791 into master will increase coverage by 0.61%.
The diff coverage is 2.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2791      +/-   ##
==========================================
+ Coverage   33.82%   34.44%   +0.61%     
==========================================
  Files         189      192       +3     
  Lines        5661     6161     +500     
  Branches      988     1124     +136     
==========================================
+ Hits         1915     2122     +207     
- Misses       3171     3396     +225     
- Partials      575      643      +68
Impacted Files Coverage Δ
editor/component-interop/index.js 0% <0%> (ø)
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
blocks/api/registration.js 100% <100%> (ø) ⬆️
editor/sidebar/post-schedule/index.js 63.15% <0%> (-3.51%) ⬇️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/index.js 0% <0%> (ø) ⬆️
editor/inserter/index.js 0% <0%> (ø) ⬆️
editor/sidebar/table-of-contents/index.js 0% <0%> (ø)
editor/sidebar/table-of-contents/item.js 0% <0%> (ø)
... and 7 more

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 95c9713...f32d649. Read the comment docs.

@effulgentsia

This comment has been minimized.

Show comment
Hide comment
@effulgentsia

effulgentsia Oct 8, 2017

Would it also be in the scope of this issue to add an API (to ComponentInterop?) for attaching event listeners to the WC DOM node to fire React synthetic events? See for example, https://staltz.com/react-could-love-web-components.html. Or is there a separate issue for that?

effulgentsia commented Oct 8, 2017

Would it also be in the scope of this issue to add an API (to ComponentInterop?) for attaching event listeners to the WC DOM node to fire React synthetic events? See for example, https://staltz.com/react-could-love-web-components.html. Or is there a separate issue for that?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Oct 9, 2017

Member

@effulgentsia At least in this specific context, since the editor won't be expected to handle custom events from the web component, I don't know that we'd need to emulate handling these to integrate with React's synthetic event system. But, in a more general use-case and depending on requirements on how the rendered component is used, I could potentially see it being part of ComponentInterop. Ideally, this would be better handled by React itself, which seems to be the topic of some ongoing conversations and effort:

Member

aduth commented Oct 9, 2017

@effulgentsia At least in this specific context, since the editor won't be expected to handle custom events from the web component, I don't know that we'd need to emulate handling these to integrate with React's synthetic event system. But, in a more general use-case and depending on requirements on how the rendered component is used, I could potentially see it being part of ComponentInterop. Ideally, this would be better handled by React itself, which seems to be the topic of some ongoing conversations and effort:

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jan 30, 2018

Member

As we move to polishing an initial release of Gutenberg, we’ve been doing some triage of old pull requests. The ideas put forth here are still valid and interesting, but simply in the name of shipping, we’re going to close this one for now. That doesn’t mean it’s not a good idea, nor that it can’t be revisited and reopened.

Member

aduth commented Jan 30, 2018

As we move to polishing an initial release of Gutenberg, we’ve been doing some triage of old pull requests. The ideas put forth here are still valid and interesting, but simply in the name of shipping, we’re going to close this one for now. That doesn’t mean it’s not a good idea, nor that it can’t be revisited and reopened.

@aduth aduth closed this Jan 30, 2018

@aduth aduth deleted the try/custom-element-interop branch Jan 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment