YUI Plugin API support
jide - October 26, 2009 - 02:03
| Project: | Wysiwyg |
| Version: | 6.x-2.x-dev |
| Component: | Editor - YUI Rich Text Editor |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Description
Hi guys,
I worked on YUI Plugin API support to make plugins that use Wysiwyg API work.
It still needs work and is a little rough, but that's an encouraging first step !
Here is an a patch to be applied against the last 6.x-2.x dev snapshot.
| Attachment | Size |
|---|---|
| wysiwyg_yui.patch | 6.42 KB |
#1
Forgot to remove some console.log() debugging.
#2
+++ wysiwyg_new/editors/js/yui.js 2009-10-26 02:46:52.000000000 +0100@@ -1,13 +1,43 @@
+ * Initialize editor instances.
+ */
+Drupal.wysiwyg.editor.init.yui = function(settings) {
+ // Initialize editor configurations.
+};
+
+/**
No need to add this if it does nothing.
+++ wysiwyg_new/editors/js/yui.js 2009-10-26 02:46:52.000000000 +0100@@ -1,13 +1,43 @@
+ for (var plugin in Drupal.settings.wysiwyg.plugins[params.format].drupal) {
+ console.log(Drupal.settings.wysiwyg.plugins[params.format].drupal[plugin]);
+ if (Drupal.settings.wysiwyg.plugins[params.format].drupal[plugin].css) {
Debug code.
+++ wysiwyg_new/editors/js/yui.js 2009-10-26 02:46:52.000000000 +0100@@ -1,13 +1,43 @@
+ // Create a group for extra plugins.
+ editor.toolbar.addButtonGroup({ group: 'plugins', buttons: [] });
We wish to handle button grouping/sorting the in a generic way for all editors, see #277954: Allow to sort editor buttons.
+++ wysiwyg_new/editors/js/yui.js 2009-10-26 02:46:52.000000000 +0100@@ -30,9 +60,57 @@ Drupal.wysiwyg.editor.detach.yui = funct
+ editor.toolbar.addButtonToGroup(button, 'plugins');
+
+ var buttons = editor.toolbar.getButtons();
+ var lastButton = buttons[buttons.length-1];
+ $(lastButton._button).parent().css('background', 'transparent url('+settings.icon+') no-repeat center');
+
See button grouping/sorting above. Is the plugin button guaranteed to be the last one?
Indents are off all over the place, use two spaces per indent.
The attach() and detach() plugin methods need to be called whenever content is set to or gotten from the editor, but you did say it needed work so I assume you knew that.
Keep it up! =)
This review is powered by Dreditor.
#3
Thanks for the review TwoD.
I am aware that this is rough and needs work, I just wanted to show some progress on this.
I'll keep working on this and will post another patch soon.
#4
I am making some progress on this, though I have difficulties to catch "onSetContent" and "onGetContent" events since there are no such events for YUI :(. I managed to implement this through different events, it does the trick but it is not as clean and straightforward as it could be. I'll digg some more.
About the grouping/sorting thing, I understand this is still a todo ? Sun speaks about some work in progress http://drupal.org/node/277954#comment-1054151. Is this still to be considered ? I may be able to help on this, but I don't want to duplicate any effort.
#5
Here we go, another patch !
Fixed in this patch :
- Overall cleanup.
- Implementation of attach and detach methods for plugins (that was not an easy one...) and isNode.
- Correction of a bug in Drupal.wysiwyg.editor.detach.yui where instance.saveHTML() was called although instance.destroy() already calls it. This caused a javascript bug and content was not saved at all.
#6
No one ? :/
#7
I haven't yet had time to actually test this but here's a review of the code itself.
I should have used a computer with larger screen for this, sorry if something got duplicated or looks malplaced.
+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100@@ -2,12 +2,37 @@
+ settings.extracss = settings.extracss+' @import "'+Drupal.settings.wysiwyg.plugins[params.format].drupal[plugin].css+'"; ';
Needs spaces around + operators.
+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100@@ -2,12 +2,37 @@
+ editor.on('toolbarLoaded', function() {
+ // Create a group for extra plugins.
+ editor.toolbar.addButtonGroup({ group: 'plugins', buttons: [] });
......
+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100
@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+ editor.toolbar.addButtonToGroup(button, 'plugins');
Like I said above, button grouping will be handled the same for all editors. This will probably mean the buttons in the settings are already sorted and grouped the way they should be when handed over to the serverside implementations, their job will be to translate these groupings into a format suitable for the editor initialization on the client. The client basically just sends the complete list of buttons to the editor init call along with the other settings. If the button commands are implemented by Drupal plugins the corresponding commands should either have been previously defined or they are also stored with the settings, depending on how the editor prefers it.
In the case of FCKeditor, all plugins are loaded during the processing of the custom configuration file, before the editor is attached to the textarea. Buttons/commands are created but not added to any toolbar as their final position in the toolbar is up to the user, and thus part of the settings. (Creating a GUI to edit that is mostly what the button-ordering issue is about.)
The buttons should be treated just like any other button and added to Drupal.settings.wysiwyg.configs.yui.format#.toolbar.buttons.0.buttons via wysiwyg_yui_settings on the server.
+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+ var lastButton = buttons[buttons.length-1];
Seems a bit risky if the above is changed and the last button is something else. In any case, missing spaces around - operator.
+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+ $(lastButton._button).parent().css('background', 'transparent url('+settings.icon+') no-repeat center');
Also needs spaces around + operators.
When not inserting buttons using this method, I think you should still be able to style them all by using the selectors mentioned on http://developer.yahoo.com/yui/examples/editor/icon_editor.html instead.
The generated css code using those selectors could probably be dynamically added to the document head as each plugin is processed.
+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+ editor.toolbar.on(plugin+'Click', function(e) {
Missing spaces.
+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+ editor._createCurrentElement('span');
Please document why this new span is needed (if creating a span is what that method does), or why an internal method is called? Just a short note is enough.
+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+ e.target.setEditorHTML(e.target._getEditorHTML());
Will this cause an extra update of the editor contents? Can it be avoided?
+ editor.on('afterSetEditorHTML', function(e) {+ if (typeof Drupal.wysiwyg.plugins[plugin].attach == 'function') {
+ e.content = Drupal.wysiwyg.plugins[plugin].attach(e.content, pluginSettings, instanceId);
+ e.content = Drupal.wysiwyg.editor.instance.yui.prepareContent(e.content);
+ }
+ });
+
+ editor.on('afterGetEditorHTML', function(e) {
+ if (typeof Drupal.wysiwyg.plugins[plugin].detach == 'function') {
+ e.content = Drupal.wysiwyg.plugins[plugin].detach(e.content, pluginSettings, instanceId);
+ }
+ });
Would it be possible to do something similar to the FCKeditor/CKEditor implementations where all plugins are iterated over in a single 'afterSetEditorHTML'/'afterGetEditorHTML' event handler? It eliminated quite a lot of flicker and delays in those editors. Could these event handlers be refactored out completely? See below.
+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+ if (button && Drupal.wysiwyg.plugins[plugin].isNode(e.target._getSelectedElement())) {
'button' is always defined here.
+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+YAHOO.widget.Editor.prototype._setEditorHTML = YAHOO.widget.Editor.prototype.setEditorHTML;
+YAHOO.widget.Editor.prototype._getEditorHTML = YAHOO.widget.Editor.prototype.getEditorHTML;
Just in case, maybe we should store the 'old' methods somewhere other than in the original object. Less risk of interfering with any other code doing the same. Probably not critical though.
+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+YAHOO.widget.Editor.prototype.setEditorHTML = function(incomingHTML) {
+ var params = { type: 'afterSetEditorHTML', target: this, content: incomingHTML };
+ this.fireEvent('afterSetEditorHTML', params);
+ this._setEditorHTML(params.content);
+};
+
+YAHOO.widget.Editor.prototype.getEditorHTML = function() {
+ var params = { type: 'afterGetEditorHTML', target: this, content: this._getEditorHTML() };
+ this.fireEvent('afterGetEditorHTML', params);
+ return params.content;
+}
Why not loop through all the plugins for the current instance here? Might save some time and cause less update-flicker if many plugins are used. It must only loop through the plugins belonging to that instance/config. These wrapper definitions could probably be moved inside the attach method. That way you get easy access to the configs via the scope chain, but it must be changed to override only a single editor instance or it will get messy.
Missing a semicolon on the last line.
+++ ../wysiwyg/editors/yui.inc 2009-10-26 02:54:06.000000000 +0100@@ -1,5 +1,5 @@
-// $Id: yui.inc,v 1.5 2009/06/09 00:18:12 sun Exp $
+// $Id: yui.inc,v 1.6.2.1 2009/10/23 03:00:56 twod Exp $
That should not be there. Edited one version and diffed against another?
+++ ../wysiwyg/editors/yui.inc 2009-10-26 02:54:06.000000000 +0100@@ -69,7 +77,7 @@ function wysiwyg_yui_version($editor) {
- if (preg_match('@version:\s([0-9\.]+)$@', $line, $version)) {
+ if (preg_match('@version:\s([0-9\.]+)@', $line, $version)) {
This was fixed in #380586: YUI editor: Version detection not working.
+++ ../wysiwyg/editors/yui.inc 2009-10-26 02:54:06.000000000 +0100@@ -187,7 +195,9 @@ function wysiwyg_yui_settings($editor, $
+ if ($plugin == 'default') {
+ $buttons[] = wysiwyg_yui_button_setting($editor, $plugin, $button, $extra);
+ }
Why the new if statement? I'm guessing this is because the buttons are added to the toolbar via JS. Please see previous comments about that.
This review is powered by Dreditor.
#8
Whops.