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: Block controls rendering by block's edit #830

Merged
merged 2 commits into from
May 26, 2017
Merged

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.

2 participants