Changes between Initial Version and Version 9 of Ticket #13771


Ignore:
Timestamp:
Oct 21, 2015, 7:05:17 AM (5 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:

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.

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #13771

    • Property Status changed from new to review_failed
    • Property Component changed from Project : CKBuilder to General
    • Property Version changed from 4.5.3 to
    • Property Milestone changed from to CKEditor 4.5.5
    • Property Owner set to Tomasz Jakut
  • Ticket #13771 – Description

    initial v9  
    1 == Steps to reproduce ==
     1Fix code is fine, while tests could definitely be a little simplified. E.g.
    22
    3 1. Download my custom build: http://ckeditor.com/builder/c21338e58b4ef94bfe0b9aff003760dd
    4 2. Unzip and `grep -r contents.css *`
    5 3. contents.css is not present in ckeditor.js
     3* 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:
    64
    7 On the contrast if downloading a standard build ckeditor.js contains a like like this: `CKEDITOR.config.contentsCss=CKEDITOR.getUrl("contents.css");`
     5        {{{
     6        'check if font size combo loads proper stylesheet': function() {
     7                var bot = this.editorBot,
     8                        name = 'FontSize';
    89
    9 == Expected result ==
     10                bot.combo( name, function( combo ) {
     11                        var isPresent = searchForStylesheet( combo );
    1012
    11 Since my build includes Rich Combo and plugins which require contents.css it is expected to be part of the build. Without contents.css all plugins based on Rich Combo use browser default font style.
     13                        assert.isTrue( isPresent, 'check if stylesheet is present in font size combo\'s iframe' );
     14                } );
     15        },
     16        }}}
    1217
     18        to:
     19
     20        {{{
     21        'check if font size combo loads proper stylesheet': function() {
     22                this.editorBot.combo( 'FontSize', function( combo ) {
     23                        var isPresent = searchForStylesheet( combo );
     24
     25                        assert.isTrue( isPresent, 'check if stylesheet is present in font size combo\'s iframe' );
     26                } );
     27        },
     28        }}}
     29
     30        `isPresent` is a different case here as
     31
     32        {{{
     33        var isPresent = searchForStylesheet( combo );
     34        assert.isTrue( isPresent, 'check if stylesheet is present in font size combo\'s iframe' );
     35        }}}
     36
     37        might actually be more readable than:
     38
     39        {{{
     40        assert.isTrue( searchForStylesheet( combo ), 'check if stylesheet is present in font size combo\'s iframe' );
     41        }}}
     42
     43        So I can see how it might improve readability - that being said I'm fine with one way or another in this particular case.
     44
     45* Stay DRY, extract `searchForStylesheet` function to `_helper/tools.js` in your tests directory.
     46* Utilize advantage of our `CKEDITOR.dom.*` wrappers, [https://github.com/cksource/ckeditor-dev/blob/t/13771/tests/tickets/13771/1.js#L62-L65 initial lines] in `searchForStylesheet` method might be simplified down to:
     47
     48        {{{
     49        var frameDoc = combo._.panel.element.getFirst().getFrameDocument(),
     50                stylesheets = frameDoc.find( 'link[rel="stylesheet"]' ).$;
     51        }}}
     52
     53        Not only that, using this you no longer need to [https://github.com/cksource/ckeditor-dev/blob/t/13771/tests/tickets/13771/1.js#L72-L73 check rel attribute].
     54
     55        Then you can actually think about rewriting it to:
     56
     57        {{{
     58        function searchForStylesheet( combo ) {
     59                var frameDoc = combo._.panel.element.getFirst().getFrameDocument();
     60                // Look for any stylesheet link elems that end with desired CSS file.
     61                return Boolean( frameDoc.findOne('link[rel="stylesheet"][href$="contents.css"]') );
     62        }
     63        }}}
     64
     65        I'd suggest you just to go through most important namespaces now:
     66
     67        * [http://docs.ckeditor.com/#!/api/CKEDITOR.dom.node CKEDITOR.dom.node]
     68        * [http://docs.ckeditor.com/#!/api/CKEDITOR.dom.element CKEDITOR.dom.element] (this time use show dropdown and exclude Inherited, as you don't need to read node methods again :))
     69        * [http://docs.ckeditor.com/#!/api/CKEDITOR.dom.event CKEDITOR.dom.event]
     70        * [http://docs.ckeditor.com/#!/api/CKEDITOR.dom.document CKEDITOR.dom.document]
     71        * [http://docs.ckeditor.com/#!/api/CKEDITOR.dom.window CKEDITOR.dom.window]
     72
     73        Then read other `CKEDITOR.dom.*` member docs the other day! :)
     74
     75* 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 [https://github.com/cksource/ckeditor-dev/blob/t/13771/tests/plugins/table/table.js#L7-L15 table plugin tests].
© 2003 – 2021 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy