#2862: plugin:fitwindow porting from v2
-------------------------------+--------------------------------------------
Reporter: garry.yao | Owner: garry.yao
Type: New Feature | Status: assigned
Priority: High | Milestone: CKEditor 3.0
Component: General | Version:
Keywords: Confirmed Review- |
-------------------------------+--------------------------------------------
Changes (by fredck):
* keywords: Confirmed Review? => Confirmed Review-
Comment:
* If we are looking to change the toolbar button name to "Maximize", then
we should also rename the plugin and command name to "maximize". It makes
more sense than "fitwindow" actually.
* I've noted that you are proposing a completely new thing here. The V2
code is already well tested and works well. I've also check the size of
each implementation and V2 gives me 2.4KB while the patch gives 3.9KB. The
extra 1.5KB brings me many doubts on the real benefit of the new approach.
* We can avoid the getTextArea function (which should be named
getTextarea instead). Actually, it's not a big issue, and even a benefit,
to have some API way to interact with the source textarea. You can add a
"editor.sourceField = textarea" right before the "editor.fire( 'mode' )"
call in the sourcearea plugin.
* The setMode function should not suffer changes. You are using that to
reload the contents. The "loadSnapshot" feature is being proposed at the
latest patch in #2763, and that should be the way to go instead.
* Generally, the strict equality operator (===) is being overused.
* Be sure to have it tested over all browsers, under strict and quirks
mode.
plugin.js:
* The file contents was duplicated for me. Martin had this issue
recently, so you may consult him.
* I still have the "\ No newline at end of file" string in this file.
There is something wrong with the patch. You will find the same thing in
the [attachment:2862_3.patch Trac preview for this patch]. Again, you
should really use TortoiseSVN. Be sure it's the best tool for SVN.
* The "onSave" and "onRestore" functions use a naming convention which
makes them look like event related functions. It looks like they can be
safely and simply named "save" and "restore".
* Moving the editor back to the position right after the replaced element
works, but, to be safer, I would save a reference to the previous node
when maximizing, and then just move the editor after it when restoring. No
one knows if that structure will have changes in the future.
* Line 14: You have CKEDITOR.document for that. Also, initialized
variables should go into separated lines:
{{{
var doc = CKEDITOR.document,
bodyEl = doc.getBody();
}}}
* Line 22: CKEDITOR.tools.extend ??? It's not needed there. Go ahead with
"this.XXX".
* Line 105: The jQuery chaining coding style is really not acceptable
here. It's also not recommended to use codes for variable names. You can
safely name that var "contents", instead of "ctEl" (and btw, reuse it at
line 110).
* Line 220: !( a === B ) !!! What about ( a !== b )? Also, you have an
unneeded "if" chain. You can do a single if there. Also (again), there is
no need to use the strict equality operator.
* Line 312: The CKEDITOR.tools.setTimeout function should be used only if
we really need one of its features. In this line we can safely use
setTimeout purely.
* Line 325: CKEDITOR.tools.extend is being overused. You can do
"CKEDITOR.config.startupFitWindow = false" directly there.
There are still a few other things that could be improved and simplified,
but we need to move forward with this. We can make a code lifting in the
future.
--
Ticket URL: <http://dev.fckeditor.net/ticket/2862#comment:9>
FCKeditor <http://www.fckeditor.net/>
The text editor for Internet
------------------------------------------------------------------------------
Open Source Business Conference (OSBC), March 24-25, 2009, San Francisco, CA
-OSBC tackles the biggest issue in open source: Open Sourcing the Enterprise
-Strategies to boost innovation and cut costs with open source participation
-Receive a $600 discount off the registration fee with the source code: SFAD
http://p.sf.net/sfu/XcvMzF8H
_______________________________________________
FCKeditor-Trac mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/fckeditor-trac
