Ticket #4606 (closed New Feature: fixed)

Opened 5 years ago

Last modified 4 years ago

AutoGrow plugin

Reported by: garry.yao Owned by: Saare
Priority: Normal Milestone: CKEditor 3.4
Component: General Version:
Keywords: Confirmed Review+ Cc:

Description

Port AutoGrow plugin to v3.

Attachments

plugin.js (1.8 KB) - added by waw325 5 years ago.
autogrow plugin
4606.patch (5.6 KB) - added by Saare 4 years ago.
4606_2.patch (6.0 KB) - added by Saare 4 years ago.
4606_3.patch (6.1 KB) - added by Saare 4 years ago.

Change History

Changed 5 years ago by waw325

autogrow plugin

comment:1 Changed 5 years ago by waw325

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 5 years ago by alfonsoml

  • Keywords HasPatch added

comment:3 Changed 5 years ago by alfonsoml

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 5 years ago by alfonsoml

  • Milestone changed from CKEditor 3.x to CKEditor 3.3

comment:5 follow-up: ↓ 6 Changed 5 years ago by waw325

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 5 years ago by waw325

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 5 years ago by fredck

  • Milestone changed from CKEditor 3.3 to CKEditor 3.4

Changed 4 years ago by Saare

comment:8 Changed 4 years ago by Saare

  • Owner set to Saare
  • Keywords Review? added; HasPatch removed
  • Status changed from new to assigned

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

comment:9 Changed 4 years ago by fredck

  • 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 4 years ago by Saare

comment:10 Changed 4 years ago by Saare

  • 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 4 years ago by Saare

comment:11 Changed 4 years ago by Saare

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

comment:12 Changed 4 years ago by fredck

  • Keywords Review+ added; Review? removed

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

comment:13 Changed 4 years ago by Saare

  • Status changed from assigned to closed
  • Resolution set to fixed

Fixed with [5687].

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