Skip to content

Conversation

@aduth
Copy link
Member

@aduth aduth commented May 17, 2017

This pull request explores an option to render a block's controls in its edit implementation.

Benefits:

  • The current controls callbacks have no access to the edit component instance, so in the case of proper component classes, they cannot make use of component state
  • Controls are not inherent to the concept of a block. More precisely they are a concern of the block's rendering in the visual editor, reflected here in the return value of edit
  • We do not need to create a separate argument signature for isActive and onClick callbacks, since it's easy enough to bind use attributes and setAttributes from the bound component instance or rendering scope.

Downsides:

  • Requires block implementer to concern themselves with focus to ensure controls are only rendered if the block is currently selected
  • It is not well-settled the extent to which we want to lean on the Slot-Fill pattern, which this uses

For related discussion, see also #717 (comment).

Open questions:

  • Would we care to have <BlockControls /> as an additional layer when the block implementer could as easily render the <Fill /> directly?
  • Since the implementation is supported already via the Formatting.Controls slot, do we care to make this the preferred approach if we could support both just the same? Could be nice to consolidate, but only if it's not viewed as more complex.
  • We could abuse React's context feature to transparently pass the block's uid from VisualEditorBlock to BlockControls directly, bypassing the need for the block implementer to concern themselves with whether the block is currently selected, at the cost of reducing transparency

Testing instructions:

Ensure that heading block controls are shown and behave the same as in master. Only the heading block has been ported in this demonstration.

@aduth aduth added [Feature] Block API API that allows to express the block paradigm. Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Question Questions about the design or development of the editor. labels May 17, 2017
@youknowriad
Copy link
Contributor

youknowriad commented May 17, 2017

I'll take a deeper look tomorrow, but here are my first thoughts about this:

  • I like the flexibility this approach gives, Controls can do more than just setting attributes and that's a big win IMO
  • Would be nice to see if we can extract the formatting controls as well from the Editable and treat them like regular controls (needs a prop to bind the editor or something like that).

Would we care to have as an additional layer when the block implementer could as easily render the directly?

I personally always prefer the flexibility and thus avoid the BlockControls component. I see us providing several reusable controls toolbars (Alignment, Block Alignment, Formatting, Styling ...) and letting the block implementor include those (and any other custom control) inside the Fill. It's seems a simple enough API for me.


I really like this proposal 👍 I'll take a deeper look tomorrow.

@youknowriad
Copy link
Contributor

@aduth Do you have any concern blocking this PR? I'm keen to get this merged and try to use this approach for everything :)

@aduth
Copy link
Member Author

aduth commented May 26, 2017

I'd hoped to solicit some more attention to this pattern, but I'm rather content with it. I'll plan to rebase / evaluate dropping BlockControls / merge tomorrow. Do you think I ought to convert existing usage now or just get the Fill in place?

@youknowriad
Copy link
Contributor

Yes, this deserves more attention IMO too. But anyway, let get this merge, I can help with converting existing controls.

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. Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Question Questions about the design or development of the editor.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants