Opened 10 years ago

Closed 10 years ago

#2869 closed New Feature (fixed)

plugin:blockoutline porting from showblock of v2

Reported by: Garry Yao Owned by: Martin Kou
Priority: Normal Milestone: CKEditor 3.0
Component: UI : Toolbar Version:
Keywords: Confirmed Review+ Cc:

Description

Related v2 tickets:

#1183
Opera: ShowBlocks doesn't display tag name in top left corner
#2525
Chrome: error if FCKConfig.StartupShowBlocks = true
#2615
Bug in FF when FCKConfig.StartupShowBlocks=true on a hidden FCKEditor instance
#2869
plugin:blockoutline porting from showblock of v2
#2983
plugin:showblock missing tagname icon
#3089
[IE]plugin:showblock problems
#3242
Showblock ignore table and list
#3774
Showblocks mode switching
#6571
[IE] showblocks doesn't bring focus back to editor with shared toolbar
#6799
ShowBlocks doesn't work with lists
#8613
showblocks confused by pathnames including spaces
#9243
Showblocks not working for divarea
#10023
ShowBlocks, do not outline forms/lists/tables
#10852
Image2: Buggy showblocks integration
#10884
Widgets integration with showblocks
#11966
[IE] Compatibility view: Caret goes outside of the last block element, when showblocks is enabled

Attachments (5)

2869.patch (6.2 KB) - added by Garry Yao 10 years ago.
2869_2.patch (5.8 KB) - added by Garry Yao 10 years ago.
Move patch to trunk
2869_3.patch (6.0 KB) - added by Garry Yao 10 years ago.
2869_4.patch (10.9 KB) - added by Martin Kou 10 years ago.
2869_5.patch (10.2 KB) - added by Martin Kou 10 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 10 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

Changed 10 years ago by Garry Yao

Attachment: 2869.patch added

comment:2 Changed 10 years ago by Garry Yao

Keywords: Confirmed Review? added

This patch also introduce a few fix to command system and plug-in init API.Beside, there's still one enhancement to be considered that we could create a sprite for the identifier images.

Changed 10 years ago by Garry Yao

Attachment: 2869_2.patch added

Move patch to trunk

comment:3 Changed 10 years ago by Garry Yao

Version: CKEditor 3.0 Beta

Changed 10 years ago by Garry Yao

Attachment: 2869_3.patch added

comment:4 Changed 10 years ago by Garry Yao

The updated patch fix the following stuff:

  1. Include part of the proposed patch on #2912 for commands state.
  2. Add startupOutline config.
  3. Fix duplicate skin loading issue.

comment:5 Changed 10 years ago by Martin Kou

Keywords: Review- added; Review? removed
Owner: changed from Garry Yao to Martin Kou
Status: assignednew

Review- due to the following reasons:

  1. There's no need to load a separate .css file for plugins now, we can use CKEDITOR.editor::addCss() for that.
  2. The constants in the blockoutline plugin are being set in the global scope, which is to be avoided.

Since Garry is focused on testing v3 now, and the changes needed should be trivial, I'm taking over this ticket.

Changed 10 years ago by Martin Kou

Attachment: 2869_4.patch added

comment:6 Changed 10 years ago by Frederico Caldeira Knabben

You didn't ask for review, by mistake I presume.

  • "Always" concatenate strings with the plus sign. The following code:
'.cke_show_blocks h6' +
'{' +
	'background-repeat: no-repeat;' +
	'border: 1px dotted gray;' +
	'padding-top: 8px;' +
	'padding-left: 8px;' +
'}'

... gets compressed like this, by our smart packager:

'.cke_show_blocks h6{background-repeat: no-repeat;border: 1px dotted gray;padding-top: 8px;padding-left: 8px;}'

You can use the [].join artifact only if you need to insert code values in the middle of the string, like ['Text', variable, 'more text'].join( '' )... of course, if we are talking about a considerable amount of text.

  • Please try to compact more the cssTemplate string with further replacements. The following strings could be replaced:
    • '.cke_show_blocks '
    • 'background-image: url(%1images/block_'
  • The command definition is to be moved outside the plugins.add function, as a private var.
  • Checking the state != CKEDITOR.TRISTATE_DISABLED in the command exec is not necessary, as this is done by the command class implementation automatically.
  • A variable caching ( this.state == CKEDITOR.TRISTATE_ON ) in exec() is not a bad idea also.
  • A variable can be assigned to editor.addCommand, avoiding the this.getCommand( 'showblocks' ) calls.

Changed 10 years ago by Martin Kou

Attachment: 2869_5.patch added

comment:7 Changed 10 years ago by Martin Kou

Keywords: Review? added; Review- removed

comment:8 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

That's great. Just to make you understand it, the compressed version of it is now 50% smaller.

If you enable the button, it's loosing the on state when switching to source and back. It was working on the previous patch. Please open a ticket for it right after committing.

comment:9 Changed 10 years ago by Martin Kou

Resolution: fixed
Status: newclosed

Fixed with [3109].

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