feature: Add ability for themes to configure font sizes; Extract fontSize logic from paragraph #6628

jorgefilipecosta · 2018-05-07T23:15:53Z

This PR depends on #6618.
It extracts the logic to manage font sizes in a similar way to what we used to abstract the color logic.
We refactored the styles, so they are not specific to paragraphs. Now, other blocks can make use of the same classes.
The class names were renamed from the format is-large-text to has-large-font-size, this makes the names consistent with the names used in color classes. The new name makes clear that the primary style set is the font-size.
Color sizes are now read from editor settings instead of being a constant in the paragraph block.

Themes can now configure the font sizes show in the editor.
Fixes: #5714

How has this been tested?

Verify changing font sizes in the paragraph still works as before, no noticeable changes are expected.
Verify we are back compatible with blocks using the old class names by pasting the following text in the code view:

<!-- wp:paragraph {"fontSize":"large"} -->
<p class="is-large-text">test</p>
<!-- /wp:paragraph -->

Add the follwing code to a theme functions.php and verify that the size picker correctly reflects it:

add_theme_support( 'editor-font-sizes', array(
    array(
        'name' => __( 'extra small', 'themeLangDomain' ),
        'shortName' => __( 'XS', 'themeLangDomain' ),
		'size' => 8,
		'slug' => 'extra-small'
    ),
    array(
        'name' => __( 'small', 'themeLangDomain' ),
        'shortName' => __( 'S', 'themeLangDomain' ),
		'size' => 12,
		'slug' => 'small'
    ),
    array(
        'name' => __( 'regular', 'themeLangDomain' ),
        'shortName' => __( 'M', 'themeLangDomain' ),
		'size' => 16,
		'slug' => 'regular'
    ),
    array(
        'name' => __( 'large', 'themeLangDomain' ),
        'shortName' => __( 'L', 'themeLangDomain' ),
		'size' => 36,
		'slug' => 'large'
    )
) );

Use Highlight Updates dev tools feature to verify no unnecessary re-renders happen when comparing to master.

aduth · 2018-07-19T12:41:54Z

This has merge conflicts which need to be resolved. I'll review what's here in the meantime.

aduth · 2018-07-19T12:43:28Z

core-blocks/paragraph/index.js

@@ -251,7 +197,7 @@ class ParagraphBlock extends Component {
 							fallbackBackgroundColor,
 							fallbackTextColor,
 						} }
-						isLargeText={ fontSize >= 18 }
+						isLargeText={ fontSize.size >= 18 }


Should this be built-in to the abstraction if "large" text (for accessibility compliancy) is a standard condition?

Yes maybe we can add a property of fontSize to contrast checker where we only pass the fontSize and it decides if it is large or not, but as it is not directly related to changes here I will do that in a different PR.

but as it is not directly related to changes here I will do that in a different PR.

Can you create a (ideally self-assigned) issue tracking this?

For this cases where I feel a PR would be very small, I prefer to create a PR directly instead of writing an issue. The PR is available at #8059.

It ended up being a little bigger than what I was expecting initally because I needed to add additional test cases.

aduth · 2018-07-19T12:45:28Z

core-blocks/paragraph/index.js

@@ -261,11 +207,12 @@ class ParagraphBlock extends Component {
 						'has-drop-cap': dropCap,
 						[ backgroundColor.class ]: backgroundColor.class,
 						[ textColor.class ]: textColor.class,
+						[ fontSize.class ]: fontSize.class,


My initial reaction without looking through to see the implementation: Why do we have both fontSize.class and getFontSizeClass?

We use fontSize.class on the edit method because there it was already computed by the HOC. On the save function we don't use the HOC so we use the getFontSizeClass.

Seems unnecessarily disjointed.

The alternatives are:
- not passing class in the HOC and use getFontSizeClass in the edit method (when editing we know class is going to be required so not passing it already is just adding more work to the developers).
- -try to use the HOC in the save (it seems unnecessary as most of its functionality is not used).- Edited: I think this is not an option because the HOC requires context and setAttributes which we don't have on save.

It seems the current version is better than the alternative we have it is also the same that we are doing in the colors logic so a change in this mechanism will require an update to the existing color mechanism so we keep the consistency.

aduth · 2018-07-19T12:46:09Z

core-blocks/paragraph/index.js

@@ -501,7 +449,7 @@ export const settings = {

 		const textClass = getColorClass( 'color', textColor );
 		const backgroundClass = getColorClass( 'background-color', backgroundColor );
-		const fontSizeClass = fontSize && `is-${ fontSize }-text`;
+		const fontSizeClass = getFontSizeClass( 'font-size', fontSize );


For what reason are we changing the class syntax now?

Do we need to update deprecated reference to the old syntax?

There are still references to is-small-text, etc. in core-blocks/paragraph/style.scss

I feel using has-large-font-size is more inline with the classes we use for colors, and I would like to change theme.

Block with the old syntax are automatically migrated as classes differences are ignored during validation.

We are keeping the old classes, to not break the existing paragraphs on the front end. I think the solution here is adding a global deprecation notice and keep the classes for two versions and I updated the code to include the notice.

aduth · 2018-07-19T12:47:34Z

editor/components/font-sizes/utils.js

+ * Returns a class based on fontSizeName.
+ *
+ * @param {string} fontSizeContext Context/place where the font size is being used, is used during class generation.
+ *                                 Normally the value is "font-size".


When would it ever not be font-size? Does this even need to be an option?

I would like to keep doors open, and maybe in the future we have a different kind of classes that need a different context e.g: relative-font-size. But its a fact that right now we don't use context at all so I will remove it for now in the future we can add it if needed.

aduth · 2018-07-19T12:48:13Z

editor/components/font-sizes/with-font-sizes.js

+ * @return {Function} Higher-order component.
+ */
+export default ( ...args ) => {
+	const fontSizeMap = reduce( args, ( fontSizeAccumulator, arg ) => {


What are we assigning here? I've no idea on a glance of the code.

chrisvanpatten · 2018-07-19T13:18:59Z

add_theme_support('editor-color-palette') was refactored in #7619 to use a single array parameter instead of multiple parameters. Maybe the same could be done here for consistency?

jorgefilipecosta · 2018-07-19T19:51:41Z

Hi @aduth thank you a lot for your review. I think I addressed most points. The remaining is the usage of getFontSizeClass in save vs fontSize.class in edit. The only solution I see is removing the class from fontSize and using getFontSizeClass in edit too. As it takes more effort from the developer I personally prefer the current approach, but the difference is not huge and if you prefer to use getFontSizeClass in both places I will gladly change the code.

Hi @chrisvanpatten the code and its documentation already used an array for fontSizes I missed the update on the PR description it is now corrected.

chrisvanpatten · 2018-07-19T19:52:33Z

@jorgefilipecosta Awesome, looks great :) Excited for this!

gziolo

I left a few comments. I have some concerns regarding the current implementation of withFontSizes HOC. At first glance it seems like we are introducig additional logic which needs happen on everyt attribute change.

gziolo · 2018-07-23T11:01:44Z

docs/extensibility/theme-support.md

+The font sizes are rendered on the font size picker in the order themes provide them.
+
+Themes are responsible for creating the classes that apply the correct font size styles.
+The class name is built appending 'has-', followed by the font size name *using* kebab case and ending in -font-size.


Nit: It should be reworderd to

ending with -font-size.

gziolo · 2018-07-23T11:02:11Z

edit-post/index.js

@@ -55,6 +55,13 @@ export function initializeEditor( id, postType, postId, settings, overridePost )
 	const reboot = reinitializeEditor.bind( null, postType, postId, target, settings, overridePost );

 	// Global deprecations which cannot otherwise be injected into known usage.
+	deprecated( 'class set is-small-text, ..., is-large-text', {
+		version: '3.5',


It's 3.6 now.

gziolo · 2018-07-23T11:04:30Z

editor/store/defaults.js

+			name: __( 'larger' ),
+			shortName: __( 'XL' ),
+			size: 48,
+			slug: 'larger',


Why not extra-large? Similar to the example where we have extra-small.

Hi @gziolo these names were chosen when configurable font named sizes were added in #5269. Changing them requires a complex deprecation logic and the gain with the change is not obvious. I updated the docs to use the same names so we are consistent.

Can't do much about it 🤷‍♂️

gziolo · 2018-07-23T11:10:22Z

editor/components/font-sizes/with-font-sizes.js

+			withSelect( ( select ) => {
+				const settings = select( 'core/editor' ).getEditorSettings();
+				return {
+					fontSizes: get( settings, [ 'fontSizes' ], DEFAULT_FONT_SIZES ),


We have fontSizes provided in settings. Do we really need this default value?

https://github.com/WordPress/gutenberg/pull/6628/files#diff-6e3d39c654369a46e3c418427b3dbddbR9 - here we don't provide the default value. We should pick one approach.

gziolo · 2018-07-23T11:19:16Z

editor/components/font-sizes/with-font-sizes.js

+					}
+
+					static getDerivedStateFromProps( { attributes, fontSizes }, previousState ) {
+						return reduce( fontSizeNames, ( newState, fontSizeAttributeName ) => {


This method will be called every time any prop changes which means every time any attribute will change. Is there any way to opimize it to avoid doing all those computations where values change for attributes that have nothing to do with fonts?

Hi @gziolo, I optimized the performance of this function. And now in the most common case where props unrelated to font size change we just compare the font size props with the previous version. Of course, the code is now a little bit more complex because of this optimization.

gziolo · 2018-07-24T10:47:24Z

editor/components/font-sizes/with-font-sizes.js

+						};
+
+						if ( ! some( fontSizeAttributeNames, didAttributesChange ) ) {
+							return previousState;


It should return null when no changes happened. See: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#new-lifecycle-getderivedstatefromprops.

The new static getDerivedStateFromProps lifecycle is invoked after a component is instantiated as well as before it is re-rendered. It can return an object to update state, or null to indicate that the new props do not require any state updates.

Corrected, I'm still mind programmed to return the same value when changes did not happen.

gziolo · 2018-07-24T11:00:09Z

editor/components/font-sizes/with-font-sizes.js

+						if ( ! some( fontSizeAttributeNames, didAttributesChange ) ) {
+							return previousState;
+						}
+


It seems like you could further simplify this code using filter method;

const changedAttributes = filter( fontSizeAttributeNames, didAttributesChange ); if ( changedAttributes.length === 0 ) { return null; } const newState = reduce( fontSizeAttributeNames, ( newStateAccumulator, customFontSizeAttributeName, fontSizeAttributeName ) => { // the same logic but without if statement }, {} ); return { ...previousState, ...newState, };

I did some code improvements following the logic you proposed. I was not able to use filter because filters always return an array but I used pickBy (is equivalent for the object case).
I kept the "some" verification, because creating a new object is still a non-trivial operation and doing the check allows us to on the most common case (font attributes did not change) do only comparisons.

gziolo

I tested it locally and everything works great. I ensured that there are no unnecessary rerenders when changing non-font attributes. Awesome job with this one.

Feel free to merge as soon as you address my last comment.

gziolo · 2018-07-26T11:08:41Z

editor/components/font-sizes/with-font-sizes.js

+/**
+ * WordPress dependencies
+ */
+import { createHigherOrderComponent, Component, compose } from '@wordpress/element';


compose and createHigherOrderComponent should be imported from @wordpress/compose - I see deprecation warning on the JS console.

…Size logic from paragraph

jorgefilipecosta added the [Type] Enhancement A suggestion for improvement. label May 7, 2018

jorgefilipecosta self-assigned this May 7, 2018

jorgefilipecosta force-pushed the update/extracted-fontsize-logic-paragraph branch 2 times, most recently from e849f06 to 0b7ad83 Compare May 7, 2018 23:27

jorgefilipecosta changed the title ~~Extract fontSize logic from paragraph and make fontSizes configurable from editor settings.~~ Add ability for themes confgure font sizes; Extract fontSize logic from paragraph May 8, 2018

jorgefilipecosta changed the title ~~Add ability for themes confgure font sizes; Extract fontSize logic from paragraph~~ Add ability for themes configure font sizes; Extract fontSize logic from paragraph May 8, 2018

jorgefilipecosta force-pushed the add/font-size-picker branch from cfda02b to 57a5512 Compare May 9, 2018 10:40

jorgefilipecosta mentioned this pull request May 9, 2018

Add FontSizePicker component and refactor paragraph block to use it. #6618

Merged

jorgefilipecosta changed the title ~~Add ability for themes configure font sizes; Extract fontSize logic from paragraph~~ Add ability for themes to configure font sizes; Extract fontSize logic from paragraph May 9, 2018

jorgefilipecosta changed the base branch from add/font-size-picker to master May 9, 2018 11:47

jorgefilipecosta force-pushed the update/extracted-fontsize-logic-paragraph branch from 3660c0e to 44fa0c4 Compare May 9, 2018 12:00

jorgefilipecosta requested a review from a team May 9, 2018 13:27

jorgefilipecosta force-pushed the update/extracted-fontsize-logic-paragraph branch from 44fa0c4 to d188bd6 Compare May 16, 2018 16:24

jorgefilipecosta force-pushed the update/extracted-fontsize-logic-paragraph branch from d188bd6 to d50f581 Compare July 3, 2018 22:25

jorgefilipecosta changed the title ~~Add ability for themes to configure font sizes; Extract fontSize logic from paragraph~~ WIP Add ability for themes to configure font sizes; Extract fontSize logic from paragraph Jul 3, 2018

jorgefilipecosta force-pushed the update/extracted-fontsize-logic-paragraph branch 2 times, most recently from de53519 to 4a4dddb Compare July 4, 2018 13:07

jorgefilipecosta changed the title ~~WIP Add ability for themes to configure font sizes; Extract fontSize logic from paragraph~~ Add ability for themes to configure font sizes; Extract fontSize logic from paragraph Jul 4, 2018

jorgefilipecosta added the [Type] Plugin Interoperability Incompatibilities between a specific plugin and the block editor. Close with workaround notes. label Jul 5, 2018

jorgefilipecosta added this to the 3.3 milestone Jul 9, 2018

aduth reviewed Jul 19, 2018

View reviewed changes

jorgefilipecosta force-pushed the update/extracted-fontsize-logic-paragraph branch 4 times, most recently from 9f3dc30 to 661c5bd Compare July 19, 2018 18:35

jorgefilipecosta modified the milestones: 3.3, 3.4 Jul 19, 2018

jorgefilipecosta mentioned this pull request Jul 20, 2018

Inline styles in paragraph when setting custom sizes in the editor #7765

Closed

gziolo added the [Priority] High Used to indicate top priority items that need quick attention label Jul 23, 2018

gziolo changed the title ~~Add ability for themes to configure font sizes; Extract fontSize logic from paragraph~~ feature: Add ability for themes to configure font sizes; Extract fontSize logic from paragraph Jul 23, 2018

jorgefilipecosta force-pushed the update/extracted-fontsize-logic-paragraph branch from 661c5bd to 6da7fe0 Compare July 23, 2018 10:57

gziolo reviewed Jul 23, 2018

View reviewed changes

jorgefilipecosta force-pushed the update/extracted-fontsize-logic-paragraph branch 4 times, most recently from a5fd9f5 to d0a4747 Compare July 23, 2018 18:10

gziolo reviewed Jul 24, 2018

View reviewed changes

jorgefilipecosta force-pushed the update/extracted-fontsize-logic-paragraph branch 2 times, most recently from 6a35a9f to 28b0e29 Compare July 24, 2018 16:34

gziolo approved these changes Jul 26, 2018

View reviewed changes

jorgefilipecosta force-pushed the update/extracted-fontsize-logic-paragraph branch 2 times, most recently from 63c2bf6 to bd46fcd Compare July 27, 2018 14:47

feature: Add ability for themes to configure font sizes; Extract font…

42f25af

…Size logic from paragraph

jorgefilipecosta force-pushed the update/extracted-fontsize-logic-paragraph branch from bd46fcd to 42f25af Compare July 27, 2018 15:26

jorgefilipecosta merged commit a975273 into master Jul 27, 2018

jorgefilipecosta deleted the update/extracted-fontsize-logic-paragraph branch July 27, 2018 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Add ability for themes to configure font sizes; Extract fontSize logic from paragraph #6628

feature: Add ability for themes to configure font sizes; Extract fontSize logic from paragraph #6628

jorgefilipecosta commented May 7, 2018 •

edited

Loading

aduth commented Jul 19, 2018

aduth Jul 19, 2018

jorgefilipecosta Jul 19, 2018

aduth Jul 19, 2018

jorgefilipecosta Jul 19, 2018 •

edited

Loading

aduth Jul 19, 2018

jorgefilipecosta Jul 19, 2018

aduth Jul 19, 2018

jorgefilipecosta Jul 19, 2018 •

edited

Loading

aduth Jul 19, 2018

jorgefilipecosta Jul 19, 2018 •

edited

Loading

aduth Jul 19, 2018

jorgefilipecosta Jul 19, 2018

aduth Jul 19, 2018

chrisvanpatten commented Jul 19, 2018

jorgefilipecosta commented Jul 19, 2018

chrisvanpatten commented Jul 19, 2018

gziolo left a comment

gziolo Jul 23, 2018

gziolo Jul 23, 2018

gziolo Jul 23, 2018

jorgefilipecosta Jul 23, 2018

gziolo Jul 24, 2018

gziolo Jul 23, 2018

gziolo Jul 23, 2018

jorgefilipecosta Jul 23, 2018

gziolo Jul 24, 2018

gziolo Jul 24, 2018

jorgefilipecosta Jul 24, 2018

gziolo Jul 24, 2018 •

edited

Loading

jorgefilipecosta Jul 24, 2018

gziolo left a comment

gziolo Jul 26, 2018

feature: Add ability for themes to configure font sizes; Extract fontSize logic from paragraph #6628

feature: Add ability for themes to configure font sizes; Extract fontSize logic from paragraph #6628

Conversation

jorgefilipecosta commented May 7, 2018 • edited Loading

How has this been tested?

aduth commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Jul 19, 2018 • edited Loading

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

jorgefilipecosta Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Jul 19, 2018 • edited Loading

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

chrisvanpatten commented Jul 19, 2018

jorgefilipecosta commented Jul 19, 2018

chrisvanpatten commented Jul 19, 2018

gziolo 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

gziolo Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented May 7, 2018 •

edited

Loading

jorgefilipecosta Jul 19, 2018 •

edited

Loading

jorgefilipecosta Jul 19, 2018 •

edited

Loading

jorgefilipecosta Jul 19, 2018 •

edited

Loading

gziolo Jul 24, 2018 •

edited

Loading