ckeditor5 icon indicating copy to clipboard operation
ckeditor5 copied to clipboard

Allow custom size unit and custom option labels in FontSizeConfig

Open Cuperino opened this issue 5 years ago • 10 comments

Feature Request

A developer may wish to use a different font-size unit, other than 'px'. They may also wish to pass custom names to the fontSize options.

In CKEditor 4 it was possible to achieve these things using the following syntax:

fontSize: '1.0/1em;1.1/1.1em;1.2/1.2em;1.3/1.3em'

I personally find repeating the unit this way redundant, considering a single unit should be used for most use cases. Which is why I propose using the following syntax:

fontSize: {
    options: [ '1.0', '1.1', '1.2', '1.3' ],
    unit: 'em'
},

Options could be strings or numbers. If no unit is specified, CKEditor would default to 'px'.

Regarding the custom option names, if the user passes strings for options, these strings could be used as the option name itself.

// This code would result in 'x' being shown to the left of the numbers in the UI.
fontSize: {
    options: [ 'x1.0', 'x1.1', 'x1.2', 'x1.3' ],
    unit: 'em'
},

Code Contribution

I needed to make sure that it was possible to use relative unit sizes in CKEditor 5, so I already went through the trouble of implementing this feature. I've published my changes at https://github.com/ImaginarySense/ckeditor5-font

Please share your feedback regarding my suggestions and the code so I can improve the code to your project's needs. I will gladly contribute my changes to CKEditor and CKSource. Where should I send the CLA to?


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

Cuperino avatar Apr 24 '19 08:04 Cuperino

cc @jodator @Reinmar

Mgsy avatar Apr 25 '19 07:04 Mgsy

This proposed solution looks OK to me. I was thinking about adding unit option also. And we had some requests about implementing them (most commonly use case pt).

As for contributing - if we decide to pursuit with this (cc @Reinmar / @mlewand) the CLA process is started on opened GH pull request and is done via email.

jodator avatar Apr 25 '19 07:04 jodator

The proposed string API change would break current behavior, where strings passed to the fontSize.options are populated as HTML classes.

As for the options.unit I think it has some uses, as it might in fact simplify config in case where you'd like to use a different unit than px.

Currently simple code might be:

options: [ '1.0', '1.1', '1.2', '1.3' ].map( val => ({
	model: val,
	title: `x${ val }`,
	view: {
		name: 'span',
		styles: {
			'font-size': `${ val }em`
		}
	}
}) )

Which is not the most readable piece.

Though I think more relevant name here is options.defaultUnit. unit alone implies that it would be used for all values no matter what, which is not true if you pass FontSizeOption.

Even though more commonly font size use the same unit, it is not always the case, so there has to be an option to use different formats. But for these rare case there's a possibility to set it using FontSizeOption definitions.

To sum it up, default unit addition would be nice, while string values doesn't look like a good proposal.

I'd love to hear from @Reinmar though.

mlewand avatar Apr 25 '19 08:04 mlewand

First of all, I totally forgot that you can pass such complex values to options as you shown @mlewand. I think no one knows that because https://ckeditor.com/docs/ckeditor5/latest/api/module_font_fontsize-FontSizeConfig.html#member-options makes it very hard to figure it out. There's no example whatsoever there. That'd be the first thing to fix – docs for config.fontSize.options should have more examples.

Second, I agree that while achieving custom units right now is doable, it's too complicated. For that we can introduce config.fontSize.unit.

Then we may have a problem with labels. How should option labels be generated. Here, the simplest solution seems to be config.fontSize.optionTitles:

// With a 1:1 array:
fontSize: {
  options: [ 1, 2, 3 ]
  optionTitles: [ '1x', '2x', '3x' ]
  unit: 'em'
}

// With a function (not necessary now, but may be useful if we'll want to introduce free-values):
fontSize: {
  options: [ 1, 2, 3 ]
  optionTitles: ( value ) => returnSomething,
  unit: 'em'
}

Finally, what if you want to handle 3 types of units at the same time? This is a really edge case, but then you can always use options together with the full-featured FontSizeOption version.

Did I miss something? :D

Reinmar avatar Apr 25 '19 09:04 Reinmar

Did I miss something? :D

Yeah, I missed something – how the proposed formats will look next to FontSizeOption... So, alternatively we could extend FontSizeOption like this:

fontSize: {
  options:  [
    {
       model: 1
       view: '1em',
       title: '1x'
    },
    {
       model: 2,
       view: '2em',
       title: '2x'
    },
    // ...
  ]
}

It requires more typing but with [ 1, 2, 3 ].map() it can be generated easily. This format certainly fits better what we have currently, but it's a bit more complicated than what I proposed in the previous comment. If we go this way, we'll have to show a couple examples in the docs of config.fontSize.options to ensure people get the gist.

Reinmar avatar Apr 25 '19 09:04 Reinmar

Don't you think that introducing unit property forces developers to use only a single unit and ignores use cases were developer wants to allow usage of a mix of different units? Why creating a limited solution?

jswiderski avatar May 11 '20 13:05 jswiderski

I thought of the unit property as a way to have a simple and clean configuration when the developer doesn't need to mix different units. The defaultUnit property suggested by @mlewand makes sense because it would suggest it's possible to mix different units and have a default when no unit is specified. @Reinmar's suggested approaches are also good because they wouldn't break existing behavior that allows mixing various units.

Now, the question arises... Which approach should be taken to make it easier to set the unit/s, without breaking the existing behavior?

Cuperino avatar May 11 '20 18:05 Cuperino

@Cuperino I know. I have read above posts. I just wanted to be that voice of reason - "don't go the limited way" ;)

jswiderski avatar May 11 '20 18:05 jswiderski

Some notes about workaround proposed by @mlewand used with supportAllValues : true property

  1. Below configuration throws error on editor load:
options: [ '5', '10', '20', '25' ].map( val => ({
		model: val,
		title: `x${ val }`,
		view: {
			name: 'span',
			styles: {
				'font-size': `${ val }pt`
			}
		}
	}) )
,supportAllValues: true
app.js:684 CKEditorError: font-size-invalid-use-of-named-presets: If config.fontSize.supportAllValues is set to true, you need to use numerical values as font size options.
  1. By changing 5 to 5.0, there is no error but applying values doesn't work:
options: [ '5.0', '10', '20', '25' ].map( val => ({
		model: val,
		title: `x${ val }`,
		view: {
			name: 'span',
			styles: {
				'font-size': `${ val }pt`
			}
		}
	}) )
,supportAllValues: true

issue3

  1. You get the best result when using unit directly next to value e.g. 10pt:
options: [ '5pt', '10pt', '20pt', '25pt' ].map( val => ({
		model: val,
		title: `x${ val }`,
		view: {
			name: 'span',
			styles: {
				'font-size': `${ val }`
			}
		}
	}) )
	,supportAllValues: true

jswiderski avatar May 22 '20 10:05 jswiderski

Any updates on when this would be availabl? Since the standard for document font size is 'pt', that seems like it should be a standard off the shelf option to have the labeled 'pt' sizes available with their equivalent 'px' behind the scenes.

Is the POC from 2022 still valid? Any updates @Reinmar ?

spacelist-ca avatar Feb 22 '24 05:02 spacelist-ca