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

Implement font size editing plugin #2275

Closed
jodator opened this issue Nov 29, 2017 · 14 comments · Fixed by ckeditor/ckeditor5-font#5
Closed

Implement font size editing plugin #2275

jodator opened this issue Nov 29, 2017 · 14 comments · Fixed by ckeditor/ckeditor5-font#5
Assignees
Labels
package:font type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@jodator
Copy link
Contributor

jodator commented Nov 29, 2017

Implement a Font Size editing plugin as part of ckeditor5-font plugin.

This issue gathers mostly requirements for editing part of that feature:

  1. it should be highly configurable in terms of markup that it outputs - either class either inline style.
  2. It should come with set of presets.
  3. The feature should be developed in the dependency injection–friendly way (see example).
  4. Schema: I think that fontSize attributes should be enabled in all contexts like paragraphs, headings, etc (so probaly inside Image caption also) (if not please comment /cc @Reinmar )
@jodator
Copy link
Contributor Author

jodator commented Nov 29, 2017

Starting from proposed configuration:

config.fontSize = {
	items: [
		{
			label: 'Tiny',
			// Used as attribute name:
			model: 'text-tiny',
			stopValue: 10,
			// Uses the ViewElementConfigDefinition interface:
			view: {
				name: 'span',
				classes: 'text-tiny'
			}
		},
		// Using styles:
		{
			label: 'Small',
			model: 'text-small',
			stopValue: 12,
			view: {
				name: 'span',
				styles: 'font-size: 12px'
			}
		}
	]
}

I'm thinking is defining sizes in pixels the only way to define sizes (I'm pretty sure that it is not the best way). A bit better in my opinion behaves relative sizing using em (or %) as this would better behave in headers that might have different size the paragraphs (or do we disallow font-size in headers?). Then you might configure sizes as:

  • tiny: font-size: 0.7em
  • small: font-size: 0.85em
  • big: font-size: 1.2em
  • huge: font-size: 1.8em

Also the CSS font-size property might accept other values as well.

The model-to-view is rather straightforward - take the defined size and that's it. The harder part is when loading existing markup to the editor and how to translate those (especially relative) values into model's attribute value.

@jodator jodator self-assigned this Nov 29, 2017
@scofalik
Copy link
Contributor

IDK if this is a correct issue as there is a few of them, but, AFAIU, stopValue configuration is mostly for V->M conversion (paste from word, copy content from website, etc.)? If so, do you have any idea how conversion between units will look like? If numeric-values are em it might be difficult to convert pasted content to them, if it was in px. And also the other way.

@jodator
Copy link
Contributor Author

jodator commented Nov 30, 2017

@scofalik yeah it's a main problem for me with this feature - how to convert those values and do we want to support them?

Probably I'll do some research on this if we can use calculated values somehow.

@Reinmar
Copy link
Member

Reinmar commented Nov 30, 2017

We can't convert them reliably if we get ems somewhere because we don't know what the base font size was (on the source page). We may only implement some heuristics to convert those to pxs.

However, this isn't crucial. We never supported that cause it was never possible. I wouldn't also worry here.

Actually, we may not even have stop values initially. I might've gone a bit too far with that. It's an option that we can test a bit later, unless you already have it implemented :D

@jodator
Copy link
Contributor Author

jodator commented Nov 30, 2017

@Reinmar I havn't implemented that yet but maybe if we put emphasis that default values should be relative then we could calculate those values somehow.

In my fiddle on this is rather simple case with getting class name based on relative font size. It might fail with default size (normal size) but this is something that I might dig into more.

@Reinmar
Copy link
Member

Reinmar commented Nov 30, 2017

You can't get computed styles during pasting or data loading. That's the problem here. We might do that inside the editor, when the content would already be there, but it'd be crazy. We'd need to allow such content into the editor first and post-fix those values after that.

Besides, if you get em values in the clipboard, then their application inside the editor won't be realistic anyway. The source page might've had completely different styles and what was 10px there will be 15px in the editor and both these values equal 1em.

@jodator
Copy link
Contributor Author

jodator commented Nov 30, 2017

Thinking more on this:

  • the relative sizes (em, %) are more intuitive for me in this feature (but maybe I don't know all cases from users),
  • I'm toward relative step sizes being default one (no px there),
  • defining step sizes with px might involve different strategy for parsing content,
  • defining step sizes with px might require one to define default font-size (if needed).

@Reinmar
Copy link
Member

Reinmar commented Nov 30, 2017

As for the feature defaulting to em... TBH, I don't know. It's an interesting idea but I don't know its implications.

One thing's for sure – in many cases people will expect pxs. So that needs to be still doable. And in case of a feature like this (highly visual) I think that developers may care more about ensuring visual consistency and stability in which case pxs work better.

So, perhaps our default styles for the font-* classes might use em while the "numeric" preset should default to px?

@jodator
Copy link
Contributor Author

jodator commented Nov 30, 2017

So I'm going to present predefined presets that will cover both:

  • px sizes - more suitable for document editor and/or PFW
  • class based sizes with em - more suitable for "semantic" editor configuration

@jodator
Copy link
Contributor Author

jodator commented Nov 30, 2017

BTW shouldn't numerical values have labels as values as well?

config.fontSize = {
	items: [
		{
			label: '10',
			// Used as attribute value:
			model: '10',
			stopValue: 10,
			// Uses the ViewElementConfigDefinition interface:
			view: {
				name: 'span',
				styles: 'font-size: 10px'
			}
		}
	]
}

Also second question as I've missed that earlier from @Reinmar's configuration that model option described as:

Used as attribute name

Shouldn't this be model's attribute value? I mean that the FontSize feature would operate on attribute named fontSize and will either set/unset it's value to currently applied font-size taken from configuration like text-tiny or even direct 10?

From description I would assume attribute named text-tiny with boolean value true?

I'm rather for fontSize attribute that will have some value set since I don't see mixing font-size attributes on one block.

@jodator
Copy link
Contributor Author

jodator commented Nov 30, 2017

Hmm.. I thinking about presets for numerical values, and it would be actually a generator:

config.fontSize = {
    items: [
        10,
        11,
        12,
        13,
        'normal',
        15,
        16,
        17,
    ]
}

// somewhere in order to not have those values defined anywhere
function generatePixelConfig( size ) {
    return {
        label: String( size ),
        model: size,
        stopValue: size,
        view: {
            name: 'span',
            styles: `font-size: ${ size }px`
        }
    }
}

This would give users a flexibility in configuration and we would not have to define "deafult" (but still very opinionated) set of presets for px based values.

@jodator
Copy link
Contributor Author

jodator commented Dec 4, 2017

As works for conversions helpers and view definitions are undergoing let's drop stopValue in MVP.

@pjasiun
Copy link

pjasiun commented Dec 5, 2017

let's drop stopValue in MVP.

I also don't like this value. I think that we may let people define numberic values of each sizes ('large': 42) or how to converted relative to absolute units (em to px), but the font size feature should take case of finding the best size to be used if one paste the content with a not supported size.

oleq referenced this issue in ckeditor/ckeditor5-font Feb 15, 2018
Feature: The initial font feature implementation. Closes #2. Closes #3. Closes #4.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-font Oct 8, 2019
@mlewand mlewand added status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:font labels Oct 8, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 8, 2019
@Ebuka-John
Copy link

Please my ckeditor loads the smallest font as the default, how do i change the default font to "big" or "huge"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:font type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants