Opened 9 years ago

Closed 9 years ago

#2820 closed New Feature (fixed)

Forms plugin

Reported by: Artur Formella Owned by: Artur Formella
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: Review+ Cc:

Description

Use of setup, commit, setupContent() and commitContent() in all dialogs in the Forms plugin

Example:

validate: function()
{
	var func = CKEDITOR.dialog.validate.integer( editor.lang.common.validateNumberFailed );
	return func.apply( this );
},
setup : function( element )
{
	this.setValue( element.getAttribute( 'rows' ) );
},
commit : function( element )
{
	element.setAttribute( 'rows', this.getValue() );
}

Attachments (6)

2820.patch (31.2 KB) - added by Artur Formella 9 years ago.
2820_2.patch (51.2 KB) - added by Artur Formella 9 years ago.
2820_3.patch (46.6 KB) - added by Artur Formella 9 years ago.
2820_4.patch (45.0 KB) - added by Artur Formella 9 years ago.
2820_5.patch (44.5 KB) - added by Artur Formella 9 years ago.
2820_6.patch (44.8 KB) - added by Artur Formella 9 years ago.

Download all attachments as: .zip

Change History (21)

Changed 9 years ago by Artur Formella

Attachment: 2820.patch added

comment:1 Changed 9 years ago by Frederico Caldeira Knabben

The patch is almost good. It misses the changes to the "select" dialog though.

Also, the reason for this changes is to have the element logic attached to the element itself. In this way, the dialog continue working even if we remove one of the elements. For that, we need to make the onShow and onOk implementations quite generic, independent of specific elements. So, calls like getContentElement must be avoided inside those functions.

I've noted that most of the dialogs are still using getContentElement inside onShow, to set the startup element focus. Other than the problem I've just described, this is something that must not be handled by the dialog definition, but by the API itself, which should automatically set the caret position.

So, a new patch should be provided without those getContentElement calls.

comment:2 Changed 9 years ago by Artur Formella

How to set initial focus?

Changed 9 years ago by Artur Formella

Attachment: 2820_2.patch added

comment:3 Changed 9 years ago by Artur Formella

Keywords: Review? added

comment:4 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

This is not a patch to trunk. Can you please recreate it?

comment:5 Changed 9 years ago by Artur Formella

Keywords: Review? added; Review- removed

In plugin.js (near line 113):

oOption = documentObject.createElement( "OPTION" );
var indexOpt = combo.getChild( index );
indexOpt.insertBeforeMe( oOption );

It doesn't work in IE and I don't know why.

Changed 9 years ago by Artur Formella

Attachment: 2820_3.patch added

comment:6 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • The CKEDITOR.plugins.forms.select definition is not required into the plugin.js file. It can safely go inside the selection dialog file.
  • You have tried to copy the original V2 code for the select tools, but it's not said that it work in the same way for V3. Things can be simplified there. The "_getObject" function usefulness is quite unclear.
  • There is an evident repetition inside the init() funciton at plugin.js. The same thing happened for the basicstyles plugin, and a solution similar to what we have there could be applied here (having a function that does the job repeatedly.
  • We don't need accesskeys for content pages of dialogs with a single page only (the "I" key is being defined for all "info" pages).

As there is a dependency on the image plugin, this one will hold on until it is not available into the trunk.

comment:7 Changed 9 years ago by Artur Formella

I made CKEDITOR.plugins.forms.select to allow developers to use it in their plugins. _getObject() provides abstraction - they could easily use this plugin to edit all select inputs (even in dialogs or on the outer page). We can also move it to CKEDITOR.dom.element.

Changed 9 years ago by Artur Formella

Attachment: 2820_4.patch added

comment:8 Changed 9 years ago by Artur Formella

Keywords: Review? added; Review- removed

comment:9 Changed 9 years ago by Artur Formella

Keywords: Review- added; Review? removed

I need to improve coding style and select dialog in IE.

Changed 9 years ago by Artur Formella

Attachment: 2820_5.patch added

comment:10 Changed 9 years ago by Artur Formella

Keywords: Review? added; Review- removed

comment:11 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • Please add the following to the init function, so the forms get visible, just like V2:
editor.addCss(
	'form' +
	'{' +
		'border: 1px dotted #FF0000;' +
		'padding: 2px;' +
	'}' );
  • You still have problems with missing "var" statements. For example:
// Patch code:
removeSelectedOptions = function ( combo )

// Correct code:
var removeSelectedOptions = function( combo )

// For private functions, even better:
function removeSelectedOptions( combo )

We'll have a Review+ after these fixes.

comment:12 Changed 9 years ago by Frederico Caldeira Knabben

Also, the form must have something inside of it when created (a <br> is ok), otherwise it gets rendered collapsed.

Changed 9 years ago by Artur Formella

Attachment: 2820_6.patch added

comment:13 Changed 9 years ago by Artur Formella

Keywords: Review? added; Review- removed

comment:14 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:15 Changed 9 years ago by Artur Formella

Resolution: fixed
Status: newclosed

Fixed with [3114]

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy