Changes between Initial Version and Version 9 of Ticket #13771

Timestamp:

Oct 21, 2015, 7:05:17 AM (9 years ago)

Author:

Marek Lewandowski

Comment:

#13771

Fix code is fine, while tests could definitely be a little simplified. E.g.

• there's no point in making name var, as it's used only once, and it does not improve readability. Same goes for bot, just compare:

'check if font size combo loads proper stylesheet': function() {
	var bot = this.editorBot,
		name = 'FontSize';

	bot.combo( name, function( combo ) {
		var isPresent = searchForStylesheet( combo );

		assert.isTrue( isPresent, 'check if stylesheet is present in font size combo\'s iframe' );
	} );
},

to:

'check if font size combo loads proper stylesheet': function() {
	this.editorBot.combo( 'FontSize', function( combo ) {
		var isPresent = searchForStylesheet( combo );

		assert.isTrue( isPresent, 'check if stylesheet is present in font size combo\'s iframe' );
	} );
},

isPresent is a different case here as

var isPresent = searchForStylesheet( combo );
assert.isTrue( isPresent, 'check if stylesheet is present in font size combo\'s iframe' );

might actually be more readable than:

assert.isTrue( searchForStylesheet( combo ), 'check if stylesheet is present in font size combo\'s iframe' );

So I can see how it might improve readability - that being said I'm fine with one way or another in this particular case.

• Stay DRY, extract searchForStylesheet function to _helper/tools.js in your tests directory.
• Utilize advantage of our CKEDITOR.dom.* wrappers, ​initial lines in searchForStylesheet method might be simplified down to:

var frameDoc = combo._.panel.element.getFirst().getFrameDocument(),
	stylesheets = frameDoc.find( 'link[rel="stylesheet"]' ).$;

Not only that, using this you no longer need to ​check rel attribute.

Then you can actually think about rewriting it to:

function searchForStylesheet( combo ) {
	var frameDoc = combo._.panel.element.getFirst().getFrameDocument();
	// Look for any stylesheet link elems that end with desired CSS file.
	return Boolean( frameDoc.findOne('link[rel="stylesheet"][href$="contents.css"]') );
}

I'd suggest you just to go through most important namespaces now:

• ​CKEDITOR.dom.node
• ​CKEDITOR.dom.element (this time use show dropdown and exclude Inherited, as you don't need to read node methods again :))
• ​CKEDITOR.dom.event
• ​CKEDITOR.dom.document
• ​CKEDITOR.dom.window

Then read other CKEDITOR.dom.* member docs the other day! :)

• It's great that you've thought about multiple editor creators! You can do it way simpler, without code duplication. Use bender.editors property, for example usage see ​table plugin tests.