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 Slot & Fill pattern for rendering Editable formatting controls #507

Merged
merged 4 commits into from
Apr 26, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 25, 2017

Related: #460
Related: #505

This pull request seeks to try an alternative pattern to address the need for communication between editable instances and the block toolbar. Maintaining and applying state changes between these components is currently quite difficult, and requires detecting changes from incoming props, and places the burden on the block implementer to ensure those props are passed along.

The Editable component is best able to manage its own internal knowledge of the current formatting state and how to apply changes, so ideally it would be solely responsible for its rendering. The challenge is in the separation between the rendered Editable and the block toolbar.

The changes here explore an idea to integrate a "Slot and Fill" pattern (see "Extensible React" React Conf 2017 video) provided through the react-slot-fill package. This is not much unlike the filter pattern in WordPress, and depending on the future stability of the react-slot-fill project and developments of JavaScript actions and filters in Core WordPress, we could consider a reimplementation using first-party filters. The application of filters to the formatting toolbar is particularly interesting since it's a likely candidate for plugin integration.

Implementation notes:

The current implementation has a few rough edges:

  • Importing Toolbar from the Editable component is awkward.
    • We may need to reorganize folders per Reorganize folders within editor #408
    • Or develop a filter pattern that allows the "fill" to provide a simple array of controls instead of the Toolbar element itself
  • Preserving focus while clicking a toolbar button is currently implemented by awkward focusOut detection using the discouraged ReactDOM.findDOMNode method
  • react-slot-fill defines an invalid peerDependency of react@^15.5.8 and logs a warning during installation (15.5.4 is the latest React available in the 15.x line)

Other ideas:

(Pardon the braindump...)

The exploration here was part of a broader effort to try to reduce the burden of using an Editable component on the block implementer. I'd started by looking through focus properties, which led me to wonder whether it would be more ideal to track editable state in our Redux store instance (@iseulde has separately explored these ideas). While this may work well for selection indices and focus, it didn't necessarily solve the problem of applying formatting. I'd considered whether a FORMAT_SELECTED action would be possible, since this would also allow formatting to be applied from anywhere in the application, but there's no easy way for the Editable component to monitor these actions as they're dispatched without overriding the store, comparing state after the action (via store.subscribe), or a global middleware which observes the action and applies the formatting, likely via tinymce.activeEditor (felt hacky and disconnected, but middleware is the appropriate place for these sorts of side effects).

Testing instructions:

Per #460, there should be no regressions in the application of inline formatting, and in displaying the active state of formatting controls for the currently selected text.

@aduth aduth added the [Feature] Blocks Overall functionality of blocks label Apr 25, 2017
@@ -9,29 +9,32 @@ import classNames from 'classnames';
import './style.scss';
import IconButton from 'components/icon-button';

function Toolbar( { controls } ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI there are no changes to this component's render logic. It needed to be converted from a stateless functional component to a class component to support ref being applied by Editable.

See: https://facebook.github.io/react/docs/refs-and-the-dom.html#refs-and-functional-components

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a comment to avoid rewriting this again.

This was referenced Apr 25, 2017
@youknowriad
Copy link
Contributor

Very interesting, I was not aware of this Slot & Fill pattern in React :). Always learning with @aduth's PRs 😄.

This seems really weird for a typical React Dev Workflow but the more I think about it, the more it seems to match perfectly what we want to achieve in this specific use-case.

this.nodes = {};

this.state = {
isFocused: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we drop the isFocued here and rely on !! this.props.focus instead?

@aduth
Copy link
Member Author

aduth commented Apr 26, 2017

Shouldn't we drop the isFocued here and rely on !! this.props.focus instead?

Yes! That was quite the oversight on my part. Using the prop simplifies the changes alot, removing the need for focus hackiness, multiple refs, findDOMNode, and Toolbar conversion.

@aduth
Copy link
Member Author

aduth commented Apr 26, 2017

This seems really weird for a typical React Dev Workflow but the more I think about it, the more it seems to match perfectly what we want to achieve in this specific use-case.

Yeah, I think it's an especially appropriate pattern in this context where we're pushing unbiased extensibility.

@youknowriad
Copy link
Contributor

I think we should continue in this way and merge this.

Though, I'm seeing a smallish bug, where sometimes you need to click two times on the control to activate it. I reproduce by focusing the end of a text block.

aduth added 4 commits April 26, 2017 15:36
Since clicking the format button will cause focus to be moved out of
the editor. Ensuring focus will cause nodeChange to be triggered with
parent according to format applied.

See: 2ef33e2
@aduth aduth force-pushed the update/editable-slot-fill branch from 3a321be to 0958fe8 Compare April 26, 2017 19:38
@aduth
Copy link
Member Author

aduth commented Apr 26, 2017

Though, I'm seeing a smallish bug, where sometimes you need to click two times on the control to activate it. I reproduce by focusing the end of a text block.

Nice catch, I guess the this.editor.focus was important after all. Restored in 0958fe8 with detailed description in the extended commit message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants