Try Slot & Fill pattern for rendering Editable formatting controls #507
Conversation
editor/components/toolbar/index.js
Outdated
| @@ -9,29 +9,32 @@ import classNames from 'classnames'; | |||
| import './style.scss'; | |||
| import IconButton from 'components/icon-button'; | |||
|
|
|||
| function Toolbar( { controls } ) { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Maybe a comment to avoid rewriting this again.
|
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. |
blocks/components/editable/index.js
Outdated
| this.nodes = {}; | ||
|
|
||
| this.state = { | ||
| isFocused: false, |
There was a problem hiding this comment.
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, |
Yeah, I think it's an especially appropriate pattern in this context where we're pushing unbiased extensibility. |
|
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. |
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
force-pushed
the
update/editable-slot-fill
branch
from
3a321be
to
0958fe8
Compare
Nice catch, I guess the |
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:
ReactDOM.findDOMNodemethodreact-slot-filldefines an invalid peerDependency ofreact@^15.5.8and logs a warning during installation (15.5.4is 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_SELECTEDaction 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 (viastore.subscribe), or a global middleware which observes the action and applies the formatting, likely viatinymce.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.