Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add class config option for font color/background color #6557

Open
kfirba opened this issue Apr 5, 2020 · 13 comments
Open

Add class config option for font color/background color #6557

kfirba opened this issue Apr 5, 2020 · 13 comments
Labels
domain:v4-compatibility This issue reports a CKEditor 4 feature/option that's missing in CKEditor 5. package:font support:2 An issue reported by a commercially licensed client. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@kfirba
Copy link

kfirba commented Apr 5, 2020

📝 Provide a description of the improvement

Hey!

We've been using CKEditor 4 for quite a while now. As part of building our website from scratch, we decided to also upgrade to CKEditor 5. The difference between the 2 versions is huge and requires some serious learning curve. That being said, we are totally into it and are investing the time needed.

One thing I could not get working is converting the fontColor and fontBackgroundColor to use classes instead of inline color code. Ideally, I would want to define my color pallete using those classes. Right now, the configuration only supports the format of {color: '#fff', label: 'White'}. What we want is to do something like: {class: 'text_white', label: 'White'}. I tried achieving this by using the conversions API, but could not make it work:

fontColor: {
          colors: [
            {color: '#000000', label: 'שחור'},
            {color: '#ffffff', label: 'לבן'},
            {color: '#18A618', label: 'ירוק'},
          ],
        },

// "teach" CKEditor how to parse `<span class="text_green"></span>`
editor.conversion.for('upcast').elementToAttribute({
    view: {
      name: 'span',
      classes: 'text_green',
    },
    model: {
      key: 'fontColor',
      value: '#18A618',
    },
  });

// Try to hack it and tell it to downcast its internal `<span color="..."></span>` to `<span class="text_green"></span>`
  editor.conversion.for('downcast').attributeToElement({
    model: {
      key: 'fontColor',
      value: '#18A618',
    },
    view: {
      name: 'span',
      classes: 'text_green',
    },
  });

How the feature works now and what you'd like to change?

Currently, we can only set inline color and background-color styles. Would like to add the styles as css classes.

  • Browser: Chrome@latest
  • OS: Linux
  • CKEditor version: 18
  • Installed CKEditor plugins: @ckeditor5/ckeditor-font

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

@kfirba kfirba added the type:improvement This issue reports a possible enhancement of an existing feature. label Apr 5, 2020
@Mgsy
Copy link
Member

Mgsy commented Apr 6, 2020

Hi, thanks for the report. I will confirm it as a feature request.

Your current conversion doesn't work, because this conversion has been already declared by Font feature. To change it, you have to override it by adding a higher priority to your converter. Just add converterPriority: 'high' to your attributeToElement methods.

@Mgsy Mgsy added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). and removed type:improvement This issue reports a possible enhancement of an existing feature. labels Apr 6, 2020
@Mgsy Mgsy changed the title Displaying font color and font background color as classes instead of inline style Add class config option for font color/background color Apr 6, 2020
@Mgsy Mgsy added this to the unknown milestone Apr 6, 2020
@kfirba
Copy link
Author

kfirba commented Apr 6, 2020

@Mgsy oh wow! My mistake was thinking it was called priority and not converterPriority! Thanks a lot!

Is there any way I can improve the workflow and not define a downcast and upcast converted for each color? Right now, it works by defining the upcast and downcast converters for each color which is quite cumbersome. Any way I can shorten it? Maybe treat it like an enum selection and execute some sort of a callback to determine the view's class?

Unrelated to this issue, but perhaps you can help me understand why this does not work: #1126 (comment)

I also tried specifying a converterPriority but it did not work, unfortunately.

@Mgsy
Copy link
Member

Mgsy commented Apr 7, 2020

Any way I can shorten it? Maybe treat it like an enum selection and execute some sort of a callback to determine the view's class?

I believe, the easiest way would be defining a key - value pairs for each color and class, looping through them and creating a conversion. I've prepared an example for you:

const classes = {
	text_green: '#18A618',
	text_blue: '#00FFFF',
	text_red: '#eb4034',
	text_yellow: '#c4bf25'
}

function AllowFontClass( editor ) {
	for ( const className in classes ) {
		editor.conversion.for( 'upcast' ).elementToAttribute( {
			view: {
			  name: 'span',
			  classes: className
			},
			model: {
			  key: 'fontColor',
			  value: classes[ className ],
			},
			converterPriority: 'high'
		  } );

		  editor.conversion.for( 'downcast' ).attributeToElement( {
			model: {
			  key: 'fontColor',
			  value: classes[ className ],
			},
			view: ( attributeValue, writer ) => {
	                    const spanElement = writer.createAttributeElement( 'span', {
					class: getClassColorName( attributeValue )
				}, { priority: 5 } );

	                    writer.setCustomProperty( 'fontClass', true, spanElement );

	                    return spanElement;
	        },
			converterPriority: 'high'
		  } );
	}
}

function getClassColorName( attributeValue ) {
	for ( const className in classes ) {
		if ( classes[ className ] == attributeValue ) {
			return className;
		}
	}

	return;
}

This is how it renders in the view:

Screenshot 2020-04-07 at 12 41 09

Remember to add AllowFontClass to your plugins array and define allowed colors (matching your classes) in your config.

Please, keep in mind this is PoC with many limitations unhandled cases and it should be treated as a starting point for your further development.

@piernik
Copy link

piernik commented Jul 31, 2020

I need that feature as well (for bootstrap based themes). Is there a chance to see it in near release?
I see that Highlight plugin does just what I want...

@Mgsy Mgsy modified the milestones: unknown, iteration 40 Jan 28, 2021
@Mgsy Mgsy modified the milestones: iteration 40, backlog Feb 4, 2021
@Reinmar Reinmar added domain:v4-compatibility This issue reports a CKEditor 4 feature/option that's missing in CKEditor 5. and removed domain:v4-compatibilty labels Mar 9, 2021
@pomek pomek removed this from the backlog milestone Feb 21, 2022
@ptmkenny
Copy link

ptmkenny commented Mar 1, 2022

This is also important for Drupal, as Drupal 10 will be on CKEditor 5 and Drupal's XSS filter strips out the style attribute automatically.

@jinkwon
Copy link

jinkwon commented Mar 16, 2022

I made simple plugin codes in my repository. It works very well in latest(v33) version.

https://github.com/jinkwon/ckeditor5-custom-color/blob/main/src/CustomFontColor.js

스크린샷 2022-03-16 오후 8 05 22

usage

import CustomFontColor, { FONT_COLORS } from "./plugins/CustomFontColor";

BalloonEditor.builtinPlugins = [
  ...
  CustomFontColor,
];

// 'light' is custom mode
const customColors = Object.keys(FONT_COLORS).map(key => {
  return {
	label: key,
	color: FONT_COLORS[key]['light'],
  }
});

BalloonEditor.defaultConfig = {
   ...
  fontColor: {
	colors: [...customColors],
  },
  fontBackgroundColor: {
	colors: [...customColors],
  },
};

@mmichaelis
Copy link

Broader Issue for several CKEditor Plugins: As our data layer does not accept any kind of style attributes, we also require representation as class attribute. As of 35.x (and before) there is some misalignment between several plugins like Styles, Font Style Plugins, Text Alignment, Image Alignment, Table (Cell) Properties, ... The first one dedicated to classes only, but misses concepts like "toggle between classes". Others provide a configuration option not to use style, but to use classes instead. And Font as well as Table styles only rely on style attribute in editing/data view.

Data Processing or Up-/Downcast? All the mapping could be done as part of data-processing at a very late stage, but the idea of providing it at data downcast (at least) seems to be the better match. So, we may give it a try.

Future? In the end, it would be good to straighten the overall approach of element styling to a common, easy to understand concept in CKEditor. With recent developments like Styles feature and GHS dedicated to CKEditor 4 migration, the chance for conflicting states increased according to some evaluations I did. It may be important for – CKEditor 6, for example – to take a deep breath and restart here.

@mmichaelis
Copy link

mmichaelis commented Nov 3, 2022

To my surprise, the adjacent features font-size and font-family already provide a configuration option, which allows using classes instead of style attributes in views:

fontSize: {
  options: [
    {
      title: "8 Pt.",
      model: "8pt",
      view: {
        name: "span",
        classes: "font-size--8",
      },
    },
  ],
},

See FontSizeOption.

It seems to be straightforward to extend the configuration option for font colors/background colors accordingly.

Side note, with reference to #2283, the option above does not support styling of menu items. If it does no harm, having duplicate styling information (we strip style tags in data-processing anyway), you could proceed as follows:

fontSize: {
  options: [
    {
      title: "8 Pt.",
      model: "8pt",
      view: {
        name: "span",
        classes: "font-size--8",
        styles: {
          "font-size": "8pt"
        },
      },
    },
  ],
},

As described in #2283, the behavior should, in the end, by aligned with Highlight or Heading feature and similar ones regarding the menu-styling.

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@rgpublic
Copy link

rgpublic commented Nov 6, 2023

Here's my comment to keep this alive. I still think it's relevant and desirable to have classes instead of just a style. First, if you have a restrictive CSP it won't work. Second, if you later need to tweak the color a bit you have to change all existing content.

@artemboyko43
Copy link

any updates here? :(

@Witoso Witoso added the support:2 An issue reported by a commercially licensed client. label Jan 29, 2024
@Teisi
Copy link

Teisi commented Feb 21, 2024

Please make it possible!

@guillome2k
Copy link

guillome2k commented Oct 5, 2024

Copy that, I have the exact same situation as the topic starter. In our tool we use color palettes. It would be nice (must have) that the font color changes also when the palette changes.

-edit
I am thinking now, maybe you can accomplish this with css vars:
[ { color: "var(--myColor1)" }, .. ]

Turns into
<span style='color:var(--myColor1)'>Text</span>

edit2:
Works as a charm for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:v4-compatibility This issue reports a CKEditor 4 feature/option that's missing in CKEditor 5. package:font support:2 An issue reported by a commercially licensed client. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

No branches or pull requests