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

Try: Components decorator extensibility pattern #1023

Closed
wants to merge 1 commit into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 5, 2017

Experimental: These changes are highly incomplete, unpolished, and break nearly as much functionality as they introduce.

This pull request seeks to explore a decorator pattern for extensibility. It is primarily composed of two parts:

  • applyDecorators, a higher-order component that flags a component as being enabled to accept decorators, responsible for managing the extensions during render.
  • registerExtension, the API by which extensions can hook to component rendering via a decorators property, an array of tuples where the first entry is the component to hook and the second entry a higher-order component to dynamically introduce to the wrapped component's rendering flow

See included wide-align usage

The use-case explored was that of wide image rendering, which is unique in that it depends on a relationship between two components: The Layout component for accessing the available width of the editor canvas, and the VisualEditorBlock component for applying the margin offsets. To achieve this I'd first explored using React context to pass information from the Layout decorator to the VisualEditorBlock component, but found that React-Redux's pure-by-default nature had the undesirable side-effect of preventing context changes from being reflected in the descendant component (#2517).

Instead I landed on the idea of namespaced extension state to be shared between all component decorators registered by registerExtension. A component decorator can interact with it using extensionState and setExtensionState from props (similar in usage to setState or setAttributes).

The approach differs from react-slot-fill in that a slot can be occupied by one or more fill to be rendered as a child, whereas a decorator wraps the original component and has power to replace or augment the original render behavior. I've not yet settled on whether the decorator pattern could serve as a complete replacement for slot/fill, or if the two complement each other in their own independent purpose. If slots were to remain, I think it could fit well within the existing API via registerExtension( { slots: { slotName: [ fillOne, fillTwo ] } } );.

Testing instructions:

As noted, there is breakage resulting from these changes, notably floated content. Observe though that Wide alignment (image, video, etc) stretches to occupy the full width.

@aduth aduth added Framework Issues related to broader framework topics, especially as it relates to javascript [Status] In Progress Tracking issues with work in progress labels Jun 5, 2017
@youknowriad
Copy link
Contributor

Mmm, good exploration, here are some initial thought/questions:

  • This is a really powerful extensibility pattern 👍
  • While it can replace Slots, sometimes using slots is more convenient (slots in the middle of render functions). I think we need time to see if we can completely replace Slots and if it's convenient enough.
  • Just a note: that both, slots and decorators kind of tie us with the UI framework for the extensions (React in this case, providing a generic adapter is probably possible though)
  • I'm not sure using this pattern to solve the "wide" alignment issue is a good thing (in its current form at least). In its current implementation, the extension supposes the align attribute serves the block alignment purpose with a "wide" value for all the blocks, which is not correct. I think if we want this to be an example on how block authors could control the parent div, we'd create an extension per block and check the blocktype here https://github.com/WordPress/gutenberg/pull/1023/files#diff-8608d58175247fbde222a7774c6d9278R32 before applying the adapter. (maybe an extension factory to avoid duplication)
  • It would be nice to surface concrete example on what can plugins do with this extensibility pattern: (add controls to all blocks, change a built-in block behaviour, ...) and document this.

Awesome job ❤️

uid: ownProps.uid,
} );
export default flow(
applyComponentDecorators,
Copy link
Member

Choose a reason for hiding this comment

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

This is exactly what we need to Collaborative Editing to be able to display a different UI state when collaborator is editing the block 👍

@aduth
Copy link
Member Author

aduth commented Nov 16, 2017

Closing for now as satisfied by #3318.

@aduth aduth closed this Nov 16, 2017
@aduth aduth deleted the add/extensions-wide-align branch November 16, 2017 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants