T/4: Initial Font plugin implementation. #5

jodator · 2018-01-10T13:20:25Z

Suggested merge commit message (convention)

Feature: Initial Font feature implementation. Closes ckeditor/ckeditor5#2275. Closes ckeditor/ckeditor5#2276. Closes ckeditor/ckeditor5#2277.

Additional information

This came with minor changes in UI/Theme required to have button in toolbar nicely rendered. I've pushed:
- https://github.com/ckeditor/ckeditor5/tree/t/ckeditor5-font/4 (docs fixes & updated mgit.json for PR review).
- https://github.com/ckeditor/ckeditor5-theme-lark/tree/t/ckeditor5-font/4.

The changes in UI/Theme changes which element gets borders and white background by default. Now it's only to dropdown button with label only - other types of buttons are rendered as buttons.

I have only one major issue with this PR which requires talks/decisions. I've used Regex to create view converter definition for Font Family as font-family inline styles might be defined in various ways in HTML - by using quotes (single/double) or by not using them. So there's a regex that checks any variation of:

Long Font Name, Other Font Name
"Long Font Name", Other Font Name
Long Font Name, 'Other Font Name'
... etc

ckeditor5-font/src/fontfamily/utils.js

Lines 58 to 98 in c77be7e

    
           // Remove quotes from font names. They will be normalized later. 
        
           const fontNames = fontDefinition.replace( /"|'/g, '' ).split( ',' ); 
        
           // The first matched font name will be used as dropdown list item title and as model value 
        
           const firstFontName = fontNames[ 0 ]; 
        
           // CSS-compatible font names. 
        
           const cssFontNames = fontNames.map( normalizeFontNameForCSS ); 
        
           // TODO: Maybe we can come with something better here? 
        
           // TODO: Also document this behavior in engine as it uses matcher~Pattern not ViewElementDefinition. 
        
           // TODO: Maybe a better solution will be a callback here? (also needs documentation) 
        
           // This will match any quote type with whitespace. 
        
           const quotesMatch = '("|\'|&qout;|\\W){0,2}'; 
        
           // Full regex will catch any style of quotation used in view. 
        
           // Example: 
        
           // from string: "Font Foo Foo, Font Bar" 
        
           // it will create a regex that will match any quotation mix: 
        
           //     - "Font Foo Foo", Font Bar 
        
           //     - 'Font Foo Foo', "Font Bar" 
        
           //     - ... etc. 
        
           const regexString = `${ quotesMatch }${ fontNames.map( n => n.trim() ).join( `${ quotesMatch },${ quotesMatch }` ) }${ quotesMatch }`; 
        
           return { 
        
           	title: firstFontName, 
        
           	model: firstFontName, 
        
           	view: { 
        
           		name: 'span', 
        
           		style: { 
        
           			'font-family': cssFontNames.join( ', ' ) 
        
           		} 
        
           	}, 
        
           	acceptsAlso: [ 
        
           		{ 
        
           			name: 'span', 
        
           			style: { 
        
           				'font-family': new RegExp( regexString ) 
        
           			} 
        
           		} 
        
           	] 
        
           };

What I can create instead is a use of a callback that would basically to what this regex but programatically. Only thing would be updating documentation in definition-based-coverters (and ViewElementDefinition, etc) to reflect this behavior.

…ipeline conversion.

…efinition interface to ckeditor5-engine.

…erters.

…buteElement as an array.

oleq

The code looks great. What we need is a better, more verbose documentation to help developers use the (sub–)features we developed.

oleq · 2018-02-12T15:07:32Z

docs/features/font-size.md

+
+{@snippet features/build-font-size-source}
+
+The {@link module:font/fontsize~FontSize} feature enables support for setting font size.


I'm afraid this is not a very rich introduction. We should clearly state here that the feature is about inline elements, classes and the style attribute.

oleq · 2018-02-12T15:18:36Z

docs/features/font-family.md

+
+{@snippet features/build-font-family-source}
+
+The {@link module:font/fontfamily~FontFamily} feature enables support for setting font family.


See my first comment in the font-size.md.

oleq · 2018-02-12T15:33:42Z

docs/features/font-size.md

+## Configuring font size options
+
+It is possible to configure which font size options are supported by the editor. Use the {@link module:font/fontsize~FontSizeConfig#options `fontSize.options`} configuration option to do so.
+Use the special keyword `'normal'` to use the default font size defined in the web page styles.


Why normal but not default? Font size defined default for the same purpose. Is this by design?

oleq · 2018-02-12T15:48:41Z

src/font.js

+ *
+ * @extends module:core/plugin~Plugin
+ */
+export default class Font extends Plugin {


This plugin has no manual test and is not even mentioned in docs. What's the purpose of it then?

oleq · 2018-02-12T15:49:27Z

src/font.js

+import FontSize from './fontsize';
+
+/**
+ * The Font plugin.


What does it do? Because it requires some classes it does not mean it enables them.

s/Font/font/

oleq · 2018-02-12T16:01:43Z

src/fontfamily/fontfamilyui.js

+import fontFamilyIcon from '../../theme/icons/font-family.svg';
+
+/**
+ * @extends module:core/plugin~Plugin


What does it do? What sort of UI does it create? How to use it in the configuration?

oleq · 2018-02-12T16:04:21Z

src/fontsize/fontsizeediting.js

+const FONT_SIZE = 'fontSize';
+
+/**
+ * The Font Size Editing feature.


What does it do? Since we break features into pieces we must explain what each piece is responsible for if developers are expected to use them.

oleq · 2018-02-12T16:04:36Z

src/fontfamily/fontfamilyediting.js

+const FONT_FAMILY = 'fontFamily';
+
+/**
+ * The Font Family Editing feature.


What does it do? Since we break features into pieces we must explain what each piece is responsible for if developers are expected to use them.

oleq · 2018-02-12T16:05:02Z

src/fontsize/fontsizeui.js

+import '../../theme/fontsize.css';
+
+/**
+ * @extends module:core/plugin~Plugin


Same as font family UI.

oleq · 2018-02-12T16:07:45Z

docs/features/font-size.md

+- `'big'`
+- `'huge'`
+
+Each size is represented in the view as a `<span>` element with the `text-*` class. For example, the `'tiny'` preset looks as follows in the editor data:


We must clearly state that we don't provide the styles. It's up to developers to style .ck-content span.tiny {} and we must show a short snippet how to do that.

oleq · 2018-02-12T16:15:34Z

I also think that this exponential approach to font previews in docs is a little bit too much

The last option is just enormous :P

jodator · 2018-02-12T16:24:43Z

The last option is just enormous :P

No it's huge ;P

Reinmar · 2018-02-12T16:30:02Z

Men... always talking about the size.

oleq · 2018-02-12T16:38:10Z

I was also wondering why these two landed so far from each other in Umberto

Reinmar · 2018-02-13T09:19:27Z

cc @m-turek

m-turek · 2018-02-13T10:18:16Z

I was also wondering why these two landed so far from each other in Umberto

That's my bad, Array.prototype.sort() doesn't preserve order of elements with same value of sorting criteria. I'll fix it soon (today)

Edit: Should work fine now if you reinstall Umberto

dkonopka · 2018-02-13T10:49:15Z

Should we change left(right) padding of every element in .ck-list to px instead of em like in heading? https://github.com/ckeditor/ckeditor5-heading/issues/63

cc @oleq

oleq · 2018-02-13T11:59:30Z

You're right @dkonopka. We should see this fixed.

jodator · 2018-02-14T16:57:09Z

I've fixed another issue in Umberto (the list view in dropdown had margin and padding set): ckeditor/ckeditor5#494 (comment).

Besides that dropdown looks OK.

jodator · 2018-02-14T17:01:38Z

@oleq & @Reinmar I've updated the docs as best I could do :) The dropdown looks OK in docs now.

…page.

…ntion as the font family feature.

jodator added 30 commits November 29, 2017 15:10

Feature: Add Font & FontSize plugins stubs.

869ede0

Feature: Add FontSizeEditing plugins stub.

75ee85f

Other: Move FontSizeEditing to font/fontsize module.

7eb6bf7

Feature: Introduce FontSizeCommand.

dbbdb19

Other: Initial implementation of FontSize configuration and editing p…

817597e

…ipeline conversion.

Other: Initial implementation of FontSize data pipeline conversion.

bd9c78c

Tests: Add font size manual test stub.

345d349

Other: Implement proper use of matchers and converters.

9b10a3f

Other: Initial support fo ViewElementDefinition interface.

35430c4

Other: Extract implementation of unified converters from ViewElementD…

db88e50

…efinition interface to ckeditor5-engine.

Other: Unify ViewElement converters for attribute and element.

419c4de

Other: Rename ViewElementDefinition attributes to plural names.

ebda823

Other: Rename AttributeElement conversion helpers.

284baf0

Other: Merge configuration defined converters into one file.

1b26cb7

Other: Update Font Size feature to latest changes in engine.

8efb27d

Tests: Reorder FontSizeCommand test.

152e48f

Other: Rename configurationdefinedconverters to definition-based-conv…

b8a9311

…erters.

Refactor: Use singular class and styles in ViewElementDefinition.

174e6ee

Added: Create commands in FontSizeEditing plugin.

58a8c85

Tests: Add 'normal' setting to tests.

62b2d9b

Other: Bootstrap FontFamilyEditing and FontFamilyCommand.

dc170d4

Tests: Add more tests for FontFamilyEditing and FontFamilyCommand.

5f3c60c

Added: Introduce FontFamily plugin.

5296f54

Tests: Added Font Family feature manual tests.

efdb954

Other: Make FontFamily work with various font-family style markup.

8e9ecd1

Other: Align FontFamilyEditing code to changes in ckeditor5-engine.

f59b1de

Added: Stub FontSizeUI plugin.

b030a9a

Changed: Pass all ConverterDefinitions to modelAttributeToViewAttri…

cad324c

…buteElement as an array.

Changed: Use one command for font features.

1543d0e

Changed: Refactor FontFamilyCommand to match FontSizeCommand.

dff41a3

oleq added 7 commits February 12, 2018 16:04

Docs: Added separators to toolbar configurations.

2f7b5ec

Docs: Improved font size and family feature guides.

1324a23

Docs: Minor improvements in docs.

4a4f47d

Docs: Minor improvements in docs.

2ccbd9c

Docs: Improvements to font package docs.

85d1fa7

Docs: Improvements to font package docs.

0691434

Docs: Improvements to font package docs.

4e063c7

oleq suggested changes Feb 12, 2018

View reviewed changes

Docs: Improved font package docs.

9f79464

Docs: Improved font package docs.

d128512

Docs: Add more explanatory descriptions.

5cf9a1e

jodator and others added 5 commits February 14, 2018 18:24

Docs: Merge font-family and font-size feature documentation into one …

fbad3d6

…page.

Docs: Various fixes and improvements in the documentation.

5929641

Changed 'normal' font size to 'default' to keep the same naming conve…

22a1946

…ntion as the font family feature.

Removed the fontsize style sheet. Moved customizations to Lark.

4e5979a

Minor lexical improvements.

33743e8

oleq merged commit a527fe7 into master Feb 15, 2018

oleq deleted the t/4 branch February 15, 2018 12:12

jodator mentioned this pull request Feb 15, 2018

Implement font size and family feature ckeditor/ckeditor5#638

Closed

Reinmar mentioned this pull request Oct 8, 2019

Handling variations of font-family notations and partial matches ckeditor/ckeditor5#2278

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/4: Initial Font plugin implementation. #5

T/4: Initial Font plugin implementation. #5

jodator commented Jan 10, 2018 •

edited by pomek

Loading

oleq left a comment

oleq Feb 12, 2018

oleq Feb 12, 2018

oleq Feb 12, 2018

oleq Feb 12, 2018

oleq Feb 12, 2018

oleq Feb 12, 2018

oleq Feb 12, 2018

oleq Feb 12, 2018

oleq Feb 12, 2018

oleq Feb 12, 2018

oleq Feb 12, 2018

oleq commented Feb 12, 2018

jodator commented Feb 12, 2018

Reinmar commented Feb 12, 2018 •

edited

Loading

oleq commented Feb 12, 2018

Reinmar commented Feb 13, 2018

m-turek commented Feb 13, 2018 •

edited

Loading

dkonopka commented Feb 13, 2018 •

edited

Loading

oleq commented Feb 13, 2018

jodator commented Feb 14, 2018

jodator commented Feb 14, 2018

	// Remove quotes from font names. They will be normalized later.
	const fontNames = fontDefinition.replace( /"\|'/g, '' ).split( ',' );

	// The first matched font name will be used as dropdown list item title and as model value
	const firstFontName = fontNames[ 0 ];

	// CSS-compatible font names.
	const cssFontNames = fontNames.map( normalizeFontNameForCSS );

	// TODO: Maybe we can come with something better here?
	// TODO: Also document this behavior in engine as it uses matcher~Pattern not ViewElementDefinition.
	// TODO: Maybe a better solution will be a callback here? (also needs documentation)
	// This will match any quote type with whitespace.
	const quotesMatch = '("\|\'\|&qout;\|\\W){0,2}';
	// Full regex will catch any style of quotation used in view.
	// Example:
	// from string: "Font Foo Foo, Font Bar"
	// it will create a regex that will match any quotation mix:
	// - "Font Foo Foo", Font Bar
	// - 'Font Foo Foo', "Font Bar"
	// - ... etc.
	const regexString = `${ quotesMatch }${ fontNames.map( n => n.trim() ).join( `${ quotesMatch },${ quotesMatch }` ) }${ quotesMatch }`;

	return {
	title: firstFontName,
	model: firstFontName,
	view: {
	name: 'span',
	style: {
	'font-family': cssFontNames.join( ', ' )
	}
	},
	acceptsAlso: [
	{
	name: 'span',
	style: {
	'font-family': new RegExp( regexString )
	}
	}
	]
	};


		{@snippet features/build-font-size-source}

		The {@link module:font/fontsize~FontSize} feature enables support for setting font size.


		{@snippet features/build-font-family-source}

		The {@link module:font/fontfamily~FontFamily} feature enables support for setting font family.

T/4: Initial Font plugin implementation. #5

T/4: Initial Font plugin implementation. #5

Conversation

jodator commented Jan 10, 2018 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

oleq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleq commented Feb 12, 2018

jodator commented Feb 12, 2018

Reinmar commented Feb 12, 2018 • edited Loading

oleq commented Feb 12, 2018

Reinmar commented Feb 13, 2018

m-turek commented Feb 13, 2018 • edited Loading

dkonopka commented Feb 13, 2018 • edited Loading

oleq commented Feb 13, 2018

jodator commented Feb 14, 2018

jodator commented Feb 14, 2018

jodator commented Jan 10, 2018 •

edited by pomek

Loading

Reinmar commented Feb 12, 2018 •

edited

Loading

m-turek commented Feb 13, 2018 •

edited

Loading

dkonopka commented Feb 13, 2018 •

edited

Loading