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

Editable: Extract the format toolbar out of the editable #923

Closed
wants to merge 4 commits into from

Conversation

youknowriad
Copy link
Contributor

This idea builds on top of #830 to allow block authors to explicitly include the formatting toolbar inside the edit function the same way they include the other controls.

Allowing the block author to explicitly include the toolbars let the block author controls the order of these toolbars. Another advantage of this PR is that the code required to apply these formats is not split between two components and the Editable component is way simpler.

I'm seeing some focus jumps (especially when using the link control), I'll try to address them asap.

Once we refactor all the blocks to use this technique, we'll be able to drop the temporary "showFormattingToolbar" prop.

A next step to this PR would be to do the same for the alignment toolbar.

@youknowriad youknowriad added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label May 29, 2017
@youknowriad youknowriad self-assigned this May 29, 2017
@youknowriad youknowriad requested a review from aduth May 29, 2017 10:21
@mtias
Copy link
Member

mtias commented May 29, 2017

Allowing the block author to explicitly include the toolbars let the block author controls the order of these toolbars.

This seems a bit problematic to me, we are making it more complicated to create a block in order to provide more flexibility. Making Editable simpler should not be the goal; the goal should be making it easier to leverage Editable for your block. We should absorbe the common tools and complexities. This change adds more burden on the block implementation, and makes it potentially harder to make changes in how formatting works across blocks.

All the setup instructions and passing this.editor in <FormatToolbar editor={ this.editor } /> seems way more convoluted than before. (Would inline toolbar still be managed via prop on Editable?)

@youknowriad
Copy link
Contributor Author

The idea here is that we could build reusable toolbars and the block author would opt-in those toolbars by including the components. Something like this:

edit: () {
  return (
   <div>
      <Controls>
          <BlockAlignmentToolbar />
          <AlignmentToolbar editable={ this.editable } />
          <FormattingToolbar editable={ this.editable } />
      </Controls>
      <Editable onSetup={ bindEditable } />
      <Editable showInlineToolbar />
    </div>
  );
}

I'm finding the controls a bit confusing now because there are different toolbars injected from different places to the same place. I'm trying to unify the way block authors adds controls to its block.

I understand the concerns about the need to bind the Editable, but I honestly find it easier to reason about than having controls appearing magically in the toolbar while others don't. I wonder if we could find a way to avoid this "editable" prop by getting the active TinyMCE instance maybe.

@mtias
Copy link
Member

mtias commented May 29, 2017

If alignment and formatting apply to the Editable components, I think it should be expressed with props. As long as those are clear, I don't see much of a problem—you get them by using Editable. In the above example it's not how the toolbar related to editable, and the binding seems like unnecessary boilerplate for most cases.

@aduth
Copy link
Member

aduth commented Jun 1, 2017

I'm finding the controls a bit confusing now because there are different toolbars injected from different places to the same place.

I feel this is a conscious decision we make when we subscribe to the concept of slot and fill. I do understand the concern though, but I'm not sure delegating this to the block implementer is the best solution, especially when the controls are to remain bound to the Editable instance anyways. I'm not sure we'll have use-cases where a user would want to remove or rearrange these controls, though if we did, (1) we could surface some Editable props to do so and (2) we might need to find a solution to Slot & Fill ordering sooner than later as a broader problem (ideally not priority 😉 ).

There's been some chatter about whether alignment should be the responsibility of a block or Editable. If it were moved back to being the responsibility of a block, then Editable is only rendering its formatting controls, which might be more reasonable to accept?

@youknowriad
Copy link
Contributor Author

Fair points here.

I thought since, List indent buttons are handled this way, table controls (v2) too and @aduth you're suggesting alignment controls. The last remaining controls are formatting. So the question is, do we want Alignment to be the special case of controls always inserted by the Editable component?

I kind of like the flexibility of having it outside but I understand that it's really tight to Editables. Thanks for your input.

@youknowriad youknowriad closed this Jun 1, 2017
@youknowriad
Copy link
Contributor Author

Rethinking about this, I think there's value in extracting all the formatting logic from the Editable into a separate component (passing an editor object) even if we include this component inside the Editable. This makes the formatting toolbar more Reusable. (I'm specifically thinking about the Freeform block and any other block using TinyMCE but not the Editable component)

@aduth
Copy link
Member

aduth commented Jun 2, 2017

Do you see that living at blocks/formatting-controls or at blocks/editable/formatting-controls ?

@youknowriad
Copy link
Contributor Author

Maybe more blocks/editable/formatting-controls, we already have editable/tinymce used by the Freeform block. Seems logic to group all TinyMCE components in one folder, but maybe change its name.

@youknowriad youknowriad mentioned this pull request Jun 5, 2017
@BoardJames
Copy link

It looks like I'm at this discussion a bit late but I think having the toolbars as separate components that are not automatically put into the slot is a great idea because as it is currently not being able to specify the toolbar order in Editable (or even turn off the basic formating tools) is really annoying.

@ntwb ntwb deleted the update/format-toolbar branch August 5, 2017 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants