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

Blocks: Try allowing custom buttons in the freeform block #1256

Closed
wants to merge 1 commit into from

Conversation

youknowriad
Copy link
Contributor

refs #794

This is just a POC and way from finished. I'm not really sure this is the path we'd want to take but this PR tries to enable custom tinymce buttons to be added in the freeform block.

  • I've created a TinymceIcon trying to match the corresponding dashicon and defaulting to the TinyMCE icon markup
  • We need to load the TinyMCE skin to allow this
  • We have several ways of defining custom buttons in TinyMCE, I've created a component for each type of button (not all of them are implemented now)
  • A global settings object containing the custom buttons and the custom mce plugin to be loaded is provided to the Editor instance

Notes

  • This is a really complex task if we want to achieve all use-cases (I'm not even sure we can). Maybe an alternative option would be to use the exact same old editor but loaded inside the freeform block, we'll loose some UI niceties I guess. (Not even sure how to do this)
  • I could use a lot of help from people already aware of how TinyMCE works internally and also from people building custom buttons for the current editor.
  • If you know widely used plugins implementing custom buttons, please try this PR. It will certainly break, but the feedback here is very important.
  • Anyone if there's an easy way to load all the code tinymce plugins js files?

@jasmussen
Copy link
Contributor

Nice, thanks for trying this. Just to emphasize — it's a nice thing to have, but if it adds a whole lot of complexity it's not a feature that has to happen.

I don't see any custom buttons, though — any steps to test?

Also, if you click the ellipsis in the freeform block, then click outside the block, do you also get a white screen or is it just me?

Regarding tinymce plugins, it seems like this would be good to look into so we can get the "add link on paste" feature, as discussed in #1041

@youknowriad
Copy link
Contributor Author

I don't see any custom buttons, though — any steps to test?

You need plugins adding custom buttons for example (TinyMCE Advanced)

Also, if you click the ellipsis in the freeform block, then click outside the block, do you also get a white screen or is it just me?

I'm not seeing this, but this is probably related

it's a nice thing to have, but if it adds a whole lot of complexity it's not a feature that has to happen.

Yes, it's complex, maybe we could limit this to simple buttons. We'll see.

@jasmussen
Copy link
Contributor

If there's a codewise clean path forward for this, it may be one of those things where someone to whom this issue is close to their hearts can shepard through. It's a nice feature, but given we eventually would suggest everyone convert their shortcodes to blocks, and add them to the inserter, it's not a blocker.

@ellatrix
Copy link
Member

Just to confirm, I think we want almost exactly the old editor as the freeform block?

@ellatrix
Copy link
Member

This is also why I encouraged not using Editable for this block, so we can initialise the old editor here as a block, with all the plugins etc. that we have now.

@youknowriad
Copy link
Contributor Author

@iseulde Yes, what you propose is the alternative I noted I suggested in the notes above and I think is worth trying in a separate PR

{ ! button.type && <SimpleButton id={ id } button={ button } editor={ editor } /> }
{ button.type === 'listbox' && <ListBoxButton id={ id } button={ button } editor={ editor } /> }
{ button.type === 'menubutton' && <MenuButton id={ id } button={ button } editor={ editor } /> }
{ button.type === 'splitbutton' && <SplitButton id={ id } button={ button } editor={ editor } /> }

Choose a reason for hiding this comment

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

There is also a colorbutton type.

if ( onPostRender ) {
button[ onPostRender ].call( {
active: ( isActive ) => this.mounted && this.setState( { isActive } ),
disabled: ( isDisabled ) => this.mounted && this.setState( { isDisabled } ),

Choose a reason for hiding this comment

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

The disabled function is not on the 'this' object but is instead on the passed event (weird I know). If you want a button that shows this then try the list indent button (using the lists plugin).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, this fixed a bug I was having with some buttons. Something like self.disabled was undefined.

Choose a reason for hiding this comment

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

Interesting! I will dig into the TinyMCE code and see if I can figure out what's going on. It will probably turn out that the 'this' object and the 'event.control' object are the same thing and that different buttons functions access them by different routes depending on the implementor.

aria-pressed={ isActive }
disabled={ isDisabled }
icon="arrow-down"
onClick={ this.toggleList }

Choose a reason for hiding this comment

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

Do you mean toggleMenu? Unless I'm missing something toggleList does not exist.

@@ -109,9 +109,16 @@ function gutenberg_register_scripts_and_styles() {

// Editor Styles.
wp_register_style(
'gutenberg-mce-skin',
includes_url( 'js/tinymce/skins/lightgray/skin.min.css' ),

Choose a reason for hiding this comment

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

If we want our own skin - for an earlier attempt on the freeform block I had converted the TinyMCE lightgray skin to scss (instead of less which TinyMCE uses). I should still have that conversion around if you want it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep could be nice to able to import the icons explicitely for example (that's what I needed here).

@BoardJames
Copy link

This has got me wondering if it might be a good idea to just create a TinyMCE theme (rather than try and hook into the editor object state after it has rendered) so I'm going to have a go at prototyping that today.

@BoardJames
Copy link

Ok, ignore my TinyMCE theme idea - my quick prototype doesn't show any advantage over just hooking events like we're already doing.

@youknowriad
Copy link
Contributor Author

I'm going to postpone the work here, if anyone wants to take over, please feel free to do. We should also try the alternative of just loading the current editor and maybe style it.

@ellatrix
Copy link
Member

@youknowriad But from how I understood it, don't we need the exact same old editor as the freeform block? I don't mind revisiting that idea, but thought it was decided like that. cc @jasmussen

@jasmussen
Copy link
Contributor

I think we need @mtias to comment also. But my understanding is that the freeform block (was just suggested renamed to "Classic Editor", which seems like it might work) will load any old content we open in the editor. And so it should support the features the old editor supported, generally. It doesn't have to support custom buttons, and it doesn't have to support every button the old editor had. But it would be nice if it did.

Whether it's a reimplementation, or a straight port, that matters less, though if we do a straight port we should do some style changes to at least make it "look at home" in the new chrome. James has been doing amazing work on the freeform block, so far, in my opinion.

One unsolved question is inserting images into the freeform block. Plan B to solve this is to have an image button on the freeform block also.

Did this answer the open questions?

@youknowriad
Copy link
Contributor Author

closing #1394 seems like a better approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants