Opened 5 years ago

Closed 5 years ago

#11532 closed New Feature (fixed)

Create editor#addContentsCss() method

Reported by: Marek Lewandowski Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.4.0
Component: General Version:
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

We should create more convenient way to add custom css files. Now we accept as editor.config.contentsCss string or array of strings which is ok, but if we want to attach to it extra css styles in plugins, we have to do some extra checking. Such code is redundant.

Method should be editor member, so we will end up with editor.addContentsCss(). Method itself should be injected from wysiwygarea plugin, since contentsCss works only for classic editor.

Method should accept as its parameter a string.

Change History (13)

comment:1 Changed 5 years ago by Piotrek Koszuliński

Description: modified (diff)
Milestone: CKEditor 4.4
Status: newconfirmed

comment:2 Changed 5 years ago by moniuch

When writing a widget, I found attaching an external stylesheet not obvious. I found the below snippet working for an iframed instance:

beforeInit: function(editor) {
        var ccss = CKEDITOR.config.contentsCss;
        if(typeof ccss == 'string'){
            ccss = [ccss];
        }
        ccss.push('/css/style.css');
        CKEDITOR.config.contentsCss = ccss;
    }

I second the vote to have a method to append reference to a css file. I think many users would appreciate also more documentation as per the appropriate handler to act in (see http://stackoverflow.com/questions/21556106/ckeditor-widget-how-do-i-attach-css-in-onload)

comment:3 Changed 5 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedreview

Pushed initial solution to t/11532 at dev and t/11532 at tests.

I belive we should still discuss whether we want to place this method only if wysiwyg plugin is added (which is current implementation).
I think it's bad because programmer needs to write extra code each time when he wants to use it (and needs to be aware of this fact):

editor.addContentsCss && editor.addContentsCss( 'assets/contents.css' )

vs simply:

editor.addContentsCss( 'assets/contents.css' )

The thing is that in inline mode it simply would not take effect.

comment:4 Changed 5 years ago by Frederico Caldeira Knabben

Yes, just like config.contentsCss doesn't have any effect on inline, we can make the API stable, always offering this method, but without any effect on inline. Ofc, mentioning this in the docs is mandatory.

comment:5 Changed 5 years ago by Piotrek Koszuliński

IMO this option is better:

editor.addContentsCss && editor.addContentsCss( 'assets/contents.css' )

because it's visible then that it's a conditional execution. The same situation as with addButton, contextMenu, etc. Note that in this case, the entire duet config.contentsCss + editor.addContentsCss will be introduced by wysiwygarea plugin. Not by core + plugin in opposite situation.

comment:6 in reply to:  5 Changed 5 years ago by Frederico Caldeira Knabben

Replying to Reinmar:

Well, considering this as a non-core feature, what you says makes sense.

comment:7 in reply to:  5 Changed 5 years ago by Marek Lewandowski

Replying to Reinmar:

Ok, so branch currently is in this state and ready for review.

comment:8 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_failed
  1. I pushed a commit fixing doc string.
  2. Method should be moved to editor's prototype. In current solution it's not available before wysiwygarea's init is executed and editor instances don't share them, so it's heavier for memory.
  3. Why checking with negation? cfg.contentsCss = !curContentsCss ? [] : [ curContentsCss ]
  4. There's a new line missing after last assertion .

comment:9 in reply to:  8 Changed 5 years ago by Marek Lewandowski

Status: review_failedreview

Replying to Reinmar:

Repushed t/11532 with fixes.

Can you apply docfix to current branch once again, please? I've mistakenly skipped pull before applying changes.

  1. Why checking with negation? cfg.contentsCss = !curContentsCss ? [] : [ curContentsCss ]

It's simplier to understand - if there's no val in curContentsCss, use empty array - otherwise wrap value within array.

comment:10 Changed 5 years ago by Piotrek Koszuliński

Can you apply docfix to current branch once again, please? I've mistakenly skipped pull before applying changes.

I force pushed my previous branch. Overriding your fixes... sorry :D I should've first think. So you need to apply them again.

It's simplier to understand - if there's no val in curContentsCss, use empty array - otherwise wrap value within array.

I could write the exact opposite. Please remove the superfluous negation.

comment:11 Changed 5 years ago by Marek Lewandowski

Changes applied. Could you take a look to updated docs:

	 * Adds given path to a stylesheet file to the exisiting {@link CKEDITOR.config#contentsCss} value.

I guess that we should still update it to sth like:

	 * Adds given stylesheet file path to the exisiting {@link CKEDITOR.config#contentsCss} value.

comment:12 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_passed

R+ and your proposal regarding docs is better, so you can change it. Remember about squashing commits.

comment:13 Changed 5 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

Merged with git:dcd4e97de870 to dev and f91f1e8e68313 at tests.

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