#2885: Implement the div container command.
-------------------------------+--------------------------------------------
Reporter: martinkou | Owner: garry.yao
Type: Task | Status: assigned
Priority: Normal | Milestone: CKEditor 3.0
Component: General | Version: SVN
Keywords: Confirmed Review- |
-------------------------------+--------------------------------------------
Changes (by martinkou):
* keywords: Confirmed Review? => Confirmed Review-
Comment:
Ok... The review took quite some time because I needed to sift through the
ticket system and previous code for precedences. Feeling like being a
lawyer here.
_source/plugins/newcontainer/plugin.js:
1. Line 32-48 - Plugin config entries must not use sub-namespaces. Refer
to #2822.
2. Line 11-30 - Now that you aren't declaring anything in the local
namespace, there's no need for the outermost closure.
3. Except for special cases like...
{{{
function abc()
{
// This return does not work in Firefox - it returns undefined.
return
{
attr : value
};
}
}}}
It is recommended that you follow the style conventions in other
[https://dev.fckeditor.net/browser/CKEditor/trunk/_source/plugins/htmlwriter/plugin.js
existing]
[https://dev.fckeditor.net/browser/CKEditor/trunk/_source/plugins/dialog/plugin.js
source]
[https://dev.fckeditor.net/browser/CKEditor/trunk/_source/plugins/keystrokes/plugin.js
files] for literal object "blocks" - i.e. The opening bracket should
occupy a new line instead of staying at the same line as the preview
token.
_source/core/dom/element.js:
1. Line 378 - The regular expression is wrong. It matches non-pure-
whitespace texts like "Hello world" or "There are some spaces in
me".
_source/core/tools.js:
1. Line 320-323 - isEmpty() is not really needed for parsing form field
values. Because when the user fills in 0, the value is the string "0"
which evaluates to true. Empty form fields automatically evaluates to
false because empty strings are false.
2. Line 329-331 - cloneArray() isn't really needed nor does it simplify
anything. Writing someArray.concat( [] ) is much shorter than
CKEDITOR.tools.cloneArray( someArray ), and is just as easy to understand.
_source/lang/en.js:
1. Lines 12-13 - Please don't remove the indenting whitespaces. If you
want to fix the styles in these two lines, there's
[https://dev.fckeditor.net/wiki/CodingStyle#Comments something else] you
can fix here.
2. Lines 375-387 - These entries are already defined in the common
strings above, please delete them from the lang file and reuse the common
strings in your dialog, so we can have less translation entries to take
care of.
3. Lines 384-387 - Please don't add unnecessary sub-namespaces inside
your lang namespace, it's only serving to increase code size without any
benefits to readability.
_source/plugins/newcontainer/dialogs/newcontainer.js:
1. Lines 214-323 - Please follow the
[https://dev.fckeditor.net/browser/CKEditor/trunk/_source/plugins/link/dialogs/link.js
current convention] for writing the arrays and object literals in dialog
files. Instead of...
{{{
children : [{ ...
}}}
You should write
{{{
children :
[
{
...
}
]
}}}
Unless you've encountered some errors like the function example above.
2. Lines 183-202: Now that we have
[https://dev.fckeditor.net/browser/CKEditor/trunk/_source/core/dom/node.js#L308
CKEDITOR.dom.node::getCommonAncestor()], you don't really need to define
this function again in your dialog. To get the common ancestor of multiple
nodes, use a loop.
3. The dialog's setup and commit phases should use the setup/commit
system introduced in [2990]. This is done in order to facilitate third
party developers to add and remove dialog fields and even tabs. Refer to
the table or the checkbox dialog in [2990] for how it is done.
--
Ticket URL: <http://dev.fckeditor.net/ticket/2885#comment:3>
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
