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 and family feature #638

Closed
jodator opened this issue Oct 26, 2017 · 20 comments
Closed

Implement font size and family feature #638

jodator opened this issue Oct 26, 2017 · 20 comments
Assignees
Labels
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 Oct 26, 2017

Similarly to the font feature I'd like to bootstrap works on font size feature.

Again this looks like another feature using configurable set of sizes but this also may be redefined. Any ideas and insights much appreciated.

@jodator
Copy link
Contributor Author

jodator commented Oct 26, 2017

I've also bootstraped a feature talks on Editor Recommendations repo: ckeditor/editor-recommendations#61.

@jodator
Copy link
Contributor Author

jodator commented Nov 3, 2017

I'd like to propose implementation details.

The feature should be developed in the dependency injection–friendly way (as in alignment feature bootstrap):

Model and data

  1. Feature name: Font Size (ckeditor5-font-size).
  2. The feature should output configurable css class names as font sizes.
  3. There should be 4 predefined settings:
    • tiny with class text-tiny,
    • small with class text-tiny,
    • big with class text-tiny,
    • huge with class text-tiny.
  4. There should be default button that will remove any font-size class (rendered as Normal option.
  5. The feature should work for inline text.
  6. Each defined font size should have its own command (like in: HeadingCommand).
  7. The feature should configurable so the users can define own sets of sizes/classes.
    ClassicEditor
    	.create( {
    		fontSize: {
    			options: [
    				{ class: 'text-tiny', title: 'Tiny' },
    				{ class: 'text-small', title: 'Small' },
    				{ class: '', title: 'normal' }, // or maybe own setting? below
    				{ class: 'text-big', title: 'Big' }
    				{ class: 'font-big', title: 'Huge' }
    			],
    			// alternate to empty class IMHO better
    			defaultFontTitle: 'Normal'
    	} )
    	.then( ... )
    	.catch( ... );

UI

  1. To be defined but probably a dropdown like in Heading feature.

@jodator
Copy link
Contributor Author

jodator commented Nov 3, 2017

☝️ @fredck & @oleq are welcome to comment :) Also as Font Family feature will be similar I'd probably use the output from this topic as a template for it.

@scofalik
Copy link
Contributor

scofalik commented Nov 3, 2017

Is there a reason to have a separate command for each size? Why is it like this in headings? /cc @oleq

@jodator
Copy link
Contributor Author

jodator commented Nov 3, 2017

@scofalik at first I thought that it must be this way to have parametrized commands. But obviously command.execute can have parameter (Dunno why I've assumed the first).

It might be as single commoand fontSize + param as defined style name.

@oleq
Copy link
Member

oleq commented Nov 13, 2017

Is there a reason to have a separate command for each size? Why is it like this in headings? /cc @oleq
👍 2

Yup. Separate commands mean modularity. Check out this discussion in ckeditor5-heading we had in the past.

@Reinmar
Copy link
Member

Reinmar commented Nov 13, 2017

Just one note – the titles of these options will need go the same way as titles of heading and image style options – they need to be translateable.

The goal is to allow developers to define this config option (so, define titles that will be used) but make the editor still use translations for them. See how it's done in the heading and image features.

@Reinmar
Copy link
Member

Reinmar commented Nov 13, 2017

BTW, since it's the 3rd use case for such feature – maybe we can find some common part of it? Like "translate this object".

@Reinmar Reinmar changed the title Implement font size feature Implement font size and family feature Nov 14, 2017
@Reinmar Reinmar added the type:feature This issue reports a feature request (an idea for a new functionality or a missing option). label Nov 14, 2017
@oleq
Copy link
Member

oleq commented Nov 23, 2017

ATM the heading feature localizes the heading.options by itself.

We could bring some automatic translation but we would need to define what should be translated first, e.g. by defininig some syntax:

getTranslatedObject( { 
    modelElement: 'paragraph', 
    title: '[t]Paragraph', 
    class: 'ck-heading_paragraph' 
} );

The method would then recursively analyze all string values in the object, figure out which should be passed through t() (string.slice( 0, 3 ) == '[t]') and make all neccessary replacements returning a cloned object.


Edit: Maybe 't(Paragraph)' instead? (no, not eval).

@ma2ciek
Copy link
Contributor

ma2ciek commented Nov 23, 2017

I'm not sure if I understand your point @oleq, but we need explicit t() calls for the translation service, for now, to be able to gather all translation keys, send them to Transifex and replace them with ids / localized strings during the Webpack's compilation. (#624, #387)

These t() calls are handled by the acorn parser, not RegExps, so both 't(Paragraph)'and '[t]Paragraph' would require some additional work in translation service.

@oleq
Copy link
Member

oleq commented Nov 24, 2017

You're right, I completely forgot. That would require lots of additional work :(

@Reinmar
Copy link
Member

Reinmar commented Nov 24, 2017

I think it can be done easily. First, you generate a hash of available translations:

const translations = {
    'Bold': t( 'Bold' ),
    'Do it!': t( 'Do it!' )
};

And then you pass it to this function as a dictionary:

const translatedObject = translateObject( { 
    title: configItem.title,
    message: configItem.message
}, translations );

This will use the translations when they are present in translations (so, when you used e.g. a predefined heading level name) and will use the value from the object if it's not present in translations.

That's what the code does now. We just need a nice helper for that.

@jodator
Copy link
Contributor Author

jodator commented Nov 27, 2017

As for UI should we go with something similar to headings? Reusing some code from headings I've got something like this:

selection_039

@Reinmar
Copy link
Member

Reinmar commented Nov 27, 2017

Feature name: Font Size (ckeditor5-font-size).

I'd rather create @ckeditor/ckeditor5-font and have there FontFamily and FontSize plugins. Just like we did with basic styles, to avoid too high granularity.

@Reinmar
Copy link
Member

Reinmar commented Nov 27, 2017

There should be 4 predefined settings:

tiny with class text-tiny,

I think that it's a bit tricky here:

  • We'll need to match those items with a pasted content (especially, in PFW's case). So, if one will define classes, perhaps they should also be able to provide font px's stop values, so that we know that "tiny" matches values up to 10px, "small" 11-14px, etc. However, I'm not sure whether this should apply to all pasted content or only content pasted from data processors. And it's not something that we must support now – let's just keep this in mind.
  • This is a kind of feature where many people won't be so happy with slightly more semantical <span class=text-tiny>. They, very often, will expect to output font-size styles and we need to allow that. Some might even expect only <small> to be available :D. So, it'd be good if all that was possible to achieve using the config. In the heading feature we already support headingItem.viewElement, so perhaps we should support here viewElement, viewClasses, viewStyles (and maybe even viewAttributes). It's worth noting that since this is a recurring thing that developers need to be able to configure features to produce the output they want, we could introduce something like ViewElementConfigDefinition interface and a helper which creates elements based on those objects (and then we could then also use it in ckeditor5-heading and solve Custom heading dropdown menu #651).
  • Finally, since configuring such a feature will not be easy (many things to specify - element names, attributes, font size stops, titles, etc.) we can have presets and preconfigured options available with a shorter syntax to use those. We already did that in the image feature where there are ImageEngine.defaultStyles. Here, we'd have presets like tiny, small, normal, big, huge, 9, 10, 11, 12, 13, etc...

There should be default button that will remove any font-size class (rendered as Normal option.

We need to think how these two features (font family and font size) will be presented in the toolbar. The "Normal" label makes no sense for the dropdown's main button (imagine two "Normal" buttons next to each other ;)). So it seems that we need some icons for both of them. But then, do we also want to show the current size/family?

Also, in terms of UI, we may consider a single dropdown for both these features but I'd rather go with separate ones to not overcomplicate the initial implementation. Especially, if we'll choose to have just normal, small icons in those dropdown's main buttons.

@Reinmar
Copy link
Member

Reinmar commented Nov 27, 2017

Regarding the view element config defition, it could work somehow like below. Note that we may already have similar interfaces in some converter builders and this should be unified as much as possible.

// Using presets:
config.fontSize = {
	items: [
		'tiny'
		'small',
		'normal',
		'big',
		'huge'
	]
}

// Using presets (px values):
config.fontSize = {
	items: [
		10
		12,
		'normal',
		16,
		20
	]
}

// Using full flavor:
config.fontSize = {
	items: [
		{
			label: 'Tiny',
			// Used as attribute name:
			model: 'text-tiny',
			stopValue: 10,
			// Uses the ViewElementConfigDefinition interface:
			view: {
				name: 'span',
				classes: 'text-tiny'
			}
		},

		{
			label: 'Small',
			model: 'text-small',
			stopValue: 12,
			view: {
				name: 'span',
				classes: 'text-small'
			}
		},

		// etc...
	]
}

// Or:
config.fontSize = {
	items: [
		{
			label: 'Tiny',
			stopValue: 10,
			view: {
				name: 'span',
				styles: 'font-size: 10px'
			}
		},

		{
			label: 'Small',
			stopValue: 12,
			view: {
				name: 'span',
				styles: 'font-size: 12px'
			}
		},

		// etc...
	]
}

Also, the heading feature requires (see #651 and https://github.com/ckeditor/ckeditor5-heading/issues/72) alternative view forms for V->M conversion (so pasting works fine):

// Or:
config.heading = {
	items: [
		{
			label: 'Heading 1',
			view: {
				name: 'h2', // renders to that
				alternatives: [
					{ name: 'h1' } // but handles also that
				]
			}
		},

		{
			label: 'Heading 2',

			view: { name: 'h3' }
		},

		// etc...
	]
}

@dkonopka
Copy link
Contributor

We need to think how these two features (font family and font size) will be presented in the toolbar. The "Normal" label makes no sense for the dropdown's main button (imagine two "Normal" buttons next to each other ;)).

Completely agree, remember that we have also heading, so it's equal to 3 dropdowns in a row, it's too much for the toolbar.

But then, do we also want to show the current size/family?

IMHO displaying enabled type like in headings it's not necessary.

We just need to properly present font-size and font-family icons.

@jodator
Copy link
Contributor Author

jodator commented Nov 29, 2017

Guys, I've created separate issues for:

Please continue talks there (especially on UI part as this might take a while ;) )

@jodator
Copy link
Contributor Author

jodator commented Feb 15, 2018

I'm closing this one as ckeditor/ckeditor5-font#5 got merged.

@jodator jodator closed this as completed Feb 15, 2018
@oleq oleq added this to the iteration 14 milestone Feb 15, 2018
@Keanhor2
Copy link

Keanhor2 commented Dec 9, 2022

Why? When I try to add fontSize and linehieght in vue js it is not work? but for other it is work.

    data: function () {
        return {
            sections: [],
            editor: ClassicEditor,
            editorConfig: {
                fontSize: {
                    options: [8,9,10,10.5,11,12,14,16,18,20]
                },
                lineHeight: {
                    options: [ 0.5, 1, 1.5, 2, 2.5,3,3.5,4,4.5,5,5.5 ]
                },
                toolbar: {
                    items: [
                        'heading',
                        'bold',
                        'italic',
                        'underline',
                        'link',
                        'undo',
                        'redo',
                        'fontSize',
                        'lineHeight'
                    ]
                },
            },
        }
    },

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

7 participants