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

Allow discovery of plugin buttons via `ae_uibridge` #667

Closed
jbalsas opened this issue Jan 9, 2017 · 7 comments
Closed

Allow discovery of plugin buttons via `ae_uibridge` #667

jbalsas opened this issue Jan 9, 2017 · 7 comments
Milestone

Comments

@jbalsas
Copy link
Member

@jbalsas jbalsas commented Jan 9, 2017

AlloyEditor supports using CKEditor plugins thanks to the ae_*bridge set of plugins. However, in order to add the buttons to a toolbar, the user needs to figure out how is the particular plugin naming the button, so for instance, in order to use the Paste from Word plugin, one would need to do the following:

var editor = AlloyEditor.editable('editable', {
    extraPlugins: AlloyEditor.Core.ATTRS.extraPlugins.value +',ae_uibridge,ae_buttonbridge,pastefromword,clipboard,dialog,dialogui',
    toolbars: {
        add: {
            buttons: ['table', 'hline', 'image', 'camera', 'PasteFromWord']
        },
        ...
    }
});

Since this is error-prone and forces the user to dig into the plugin code for the exact string, it would be nice to have some kind of API so a user could simply do:

var editor = AlloyEditor.editable('editable', {
    extraPlugins: AlloyEditor.Core.ATTRS.extraPlugins.value +',ae_uibridge,ae_buttonbridge,pastefromword,clipboard,dialog,dialogui',
    toolbars: {
        add: {
            buttons: ['table', 'hline', 'image', 'camera', ...AlloyEditor.getButtonsFor('pastefromword')]
        },
        ...
    }
});
@jbalsas jbalsas added this to the 1.3.1 milestone Jan 9, 2017
@jbalsas jbalsas removed this from the 1.3.1 milestone Jan 13, 2017
@jbalsas
Copy link
Member Author

@jbalsas jbalsas commented Jan 13, 2017

This won't make it in the 1.3.1 milestone. Targeting next release now...

@jbalsas jbalsas added this to the 1.3.2 milestone Jan 19, 2017
@julien
Copy link
Member

@julien julien commented Jan 23, 2017

Hi @jbalsas,

Could you tell me if something like this

Would be a possible implementation?

Thanks!

@jbalsas
Copy link
Member Author

@jbalsas jbalsas commented Jan 23, 2017

Hey @julien, thanks for working on this. Unfortunately, I don't think that's a viable implementation. We need these to be dynamic in a way that only available buttons show up and in a way we don't have to manually maintain a plugin list.

On top of that, there are some plugins exposing more than one ui component. For example the font plugin introduces two buttons Font and FontSize

The expected behaviour would be:

// Scenario 1, font plugin is loaded using extraPlugins
editor.getButtonsFor('font'); // ['Font', 'FontSize']
// Scenario 2, font plugin is not loaded
editor.getButtonsFor('font'); // []

I think the common entry point is https://github.com/liferay/alloy-editor/blob/master/src/ui/react/src/uibridge/uibridge.js#L27-L35, or inside the independent add handlers of every bridge. Plugins registering ui components are necessarily running over those handlers, and if we could identify the calling plugin, then we'd have a dynamic and real map of the available buttons.

@julien
Copy link
Member

@julien julien commented Jan 24, 2017

Hi @jbalsas you're welcome, and thanks for the pointers I will have a look in that direction.

@julien
Copy link
Member

@julien julien commented Jan 24, 2017

Hi again @jbalsas,

So I had a look at https://github.com/liferay/alloy-editor/blob/master/src/ui/react/src/uibridge/uibridge.js#L27-L35 and it was a good starting point.

I haven't prepared any pr yet, but as I understand we need some kind of "iterator" to go through all the "required" plugins given a plugin name ... (please correct me if I'm wrong)

I've modified my previous code to look like this

getButtonFor: function (name) {
    
    function PluginIterator(plugin) {
        this.plugin = plugin;
    }
    
    PluginIterator.prototype.hasNext = function () {
        return this.plugin && this.plugin.requires;
    };
    
    PluginIterator.prototype.next = function () {
        var plugin = CKEDITOR.plugins.get(this.plugin.requires);
        if (plugin) {
            this.plugin = plugin;
        }
        return this.plugin.name;
    };

    var plugin = CKEDITOR.plugins.get(name.toLowerCase());
    if (!plugin) {
        return '';
    }

    var iterator = new PluginIterator(plugin);
    var requires = [plugin.name];
    while (iterator.hasNext()) {
        requires.push(iterator.next());
    }
    return requires;
}

Which would work the following way

AlloyEditor.getButtonFor('PasteFromWord');
// output: ["pastefromword", "clipboard", "dialog", "dialogui"]

// or in the case of an unexisting plugin

AlloyEditor.getButtonFor('NoSuchPluginRegistered');
// output: ''

The PluginIterator might not be the best name and should probably be placed in another file (or it's own file) rather than being in the getButtonFor function, but tell me if that looks better than previously.

Thanks!

@jbalsas
Copy link
Member Author

@jbalsas jbalsas commented Jan 25, 2017

Hey @julien, you're overthinking this. Take the font plugin as an example.

You can see that it will call the editor.ui.addRichCombo method one per registered ui component.

In there, we already have the buttons names, so we could simply do:

var pluginName = /plugins\/(.*)\/plugin.js/.exec((new Error().stack))[1];

AlloyEditor.BRIDGE_BUTTONS[pluginName] = AlloyEditor.BRIDGE_BUTTONS[pluginName].push(richComboName);

Then, you should simply need something like:

AlloyEditor.getButtonFor = function(pluginName) {
    return AlloyEditor.BRIDGE_BUTTONS[pluginName] || [];
}

Now, of course throwing an error to get the plugin name sounds like a very bad idea, so it'd be better to be able to do it in a nicer way :)

julien added a commit to julien/alloy-editor that referenced this issue Jan 26, 2017
julien added a commit to julien/alloy-editor that referenced this issue Jan 26, 2017
julien added a commit to julien/alloy-editor that referenced this issue Feb 20, 2017
julien added a commit to julien/alloy-editor that referenced this issue Feb 20, 2017
@jbalsas jbalsas removed this from the 1.3.2 milestone Apr 4, 2017
jbalsas added a commit that referenced this issue Apr 5, 2017
@jbalsas jbalsas added this to the 1.3.2 milestone Apr 6, 2017
@jbalsas
Copy link
Member Author

@jbalsas jbalsas commented Apr 6, 2017

I've re-added this to the now 1.4.0 milestone. Based on #718 I'm confident we can land this soon....

jbalsas added a commit that referenced this issue Apr 6, 2017
jbalsas added a commit that referenced this issue Apr 6, 2017
jbalsas added a commit that referenced this issue Apr 6, 2017
jbalsas added a commit that referenced this issue Apr 6, 2017
jbalsas added a commit that referenced this issue Apr 6, 2017
jbalsas added a commit that referenced this issue Apr 6, 2017
jbalsas added a commit that referenced this issue Apr 6, 2017
Fixes #667
@ipeychev ipeychev closed this in 728d091 Apr 6, 2017
ipeychev added a commit that referenced this issue Apr 6, 2017
ipeychev added a commit that referenced this issue Apr 6, 2017
ipeychev added a commit that referenced this issue Apr 6, 2017
ipeychev added a commit that referenced this issue Apr 6, 2017
ipeychev added a commit that referenced this issue Apr 6, 2017
ipeychev added a commit that referenced this issue Apr 6, 2017
ipeychev added a commit that referenced this issue Apr 6, 2017
Fixes #667
ipeychev added a commit that referenced this issue Apr 6, 2017
Fixes #667
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants