Opened 8 years ago

Closed 7 years ago

#4606 closed New Feature (fixed)

AutoGrow plugin

Reported by: Garry Yao Owned by: Sa'ar Zac Elias
Priority: Normal Milestone: CKEditor 3.4
Component: General Version:
Keywords: Confirmed Review+ Cc:

Description

Port AutoGrow plugin to v3.

Attachments (4)

plugin.js (1.8 KB) - added by Wesley Walser 8 years ago.
autogrow plugin
4606.patch (5.6 KB) - added by Sa'ar Zac Elias 7 years ago.
4606_2.patch (6.0 KB) - added by Sa'ar Zac Elias 7 years ago.
4606_3.patch (6.1 KB) - added by Sa'ar Zac Elias 7 years ago.

Download all attachments as: .zip

Change History (17)

Changed 8 years ago by Wesley Walser

Attachment: plugin.js added

autogrow plugin

comment:1 Changed 8 years ago by Wesley Walser

I have attached my swipe at implementing the autogrow plugin. This currently works for me and I have taken the time to make it meet the CKE coding standard but I do have questions.

These is a getResizeable function exposed by some themes that is used by the resizeable plugin that comes with CKE, and there is a setHeight function exposed by the editor itself that is used in some plugins. Should I use either of those functions as opposed to the method that I use in my code? If so, which one?

Anything else stand out to anyone on the CKE team?

Thanks, Wesley Walser

comment:2 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Keywords: HasPatch added

comment:3 Changed 8 years ago by Alfonso Martínez de Lizarrondo

These are my personal opinions:

I don't see a editor.setHeight function, only editor.resize(width, height, isContentHeight, resizeInner) provided by the default theme. In its current status it doesn't provide any extra benefit over directly setting the height if I'm right, but you should fire also the 'resize' event. The question is what would happen if someone creates a very different theme

I think that the default value for CKEDITOR.config.autogrowMaxHeight should be left empty as in the previous plugin.

Besides those points, another question is the correct location for this plugin. I guess that it should be placed like the rest in the plugins folder, but not included in the default 'plugins' config entry, and a sample file should be provided to show how it works.

comment:4 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Milestone: CKEditor 3.xCKEditor 3.3

comment:5 Changed 8 years ago by Wesley Walser

Thanks Gary,

I will add the resize event and the change to the default MaxHeight soon and submit another patch.

Thanks for the feedback.

comment:6 in reply to:  5 Changed 8 years ago by Wesley Walser

Replying to waw325:

Thanks Garry,

I will add the resize event and the change to the default MaxHeight soon and submit another patch.

Thanks for the feedback.

Oops, thanks to alfonsoml. I just assume Garry was the one commenting.

comment:7 Changed 8 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.3CKEditor 3.4

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 4606.patch added

comment:8 Changed 7 years ago by Sa'ar Zac Elias

Keywords: Review? added; HasPatch removed
Owner: set to Sa'ar Zac Elias
Status: newassigned

The patch adds the new plugin with a sample, targeting Alfonso's suggestions.

comment:9 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • We're trying to have configuration options a "documentation only", avoiding initializing them in the code (as you're doing at lines 59 and 69).
  • It's nice to have a dedicated sample for this one, but it needs to be listed in the samples/index.html file also.

Let's go ahead with the 100ms delay thing and see the users feedback.

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 4606_2.patch added

comment:10 Changed 7 years ago by Sa'ar Zac Elias

Keywords: Review? added; Review- removed

There's no reason to have the same check for the max value, as it should be zero by default.

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 4606_3.patch added

comment:11 Changed 7 years ago by Sa'ar Zac Elias

4606_2.patch has some leftovers in it, please review 4606_3 instead.

comment:12 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

Great job! Please commit this one in the 3.4.x branch.

comment:13 Changed 7 years ago by Sa'ar Zac Elias

Resolution: fixed
Status: assignedclosed

Fixed with [5687].

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