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

Packages: Refactor Common Block Code #16229

Closed
wants to merge 3 commits into from

Conversation

epiqueras
Copy link
Contributor

@epiqueras epiqueras commented Jun 20, 2019

cc @youknowriad

Relates to #15450

Description

This PR begins to explore a new way to share Common Block Code using React hooks.

In @wordpress/block-editor:

It introduces a low level hook, useAttributePicker, that is very flexible and can be easily used to build higher level hooks like useColor and useCustomColor which replaced the previous implementation of withColors and createCustomColorsHOC with a fraction of the code.

use-colors.js

export function useCustomColors( colorTypes, colorPalette ) {
	const attributePicker = useAttributePicker( {
		names: colorTypes.flatMap( ( colorType ) =>
			typeof colorType === 'string' ? colorType : Object.keys( colorType )
		),
		valuesEnum: colorPalette,

		findEnumValue: ( colorPaletteObject, newColorValue ) =>
			colorPaletteObject.color === newColorValue,
		mapEnumValue: ( colorPaletteObject ) => colorPaletteObject.slug,

		mapAttribute: ( colorSlugOrCustomValue, name ) => {
			const foundColorPaletteObject = colorPalette.find(
				( colorPaletteObject ) => colorPaletteObject.slug === colorSlugOrCustomValue
			) || { color: colorSlugOrCustomValue };

			let colorClass;
			if ( foundColorPaletteObject.slug ) {
				const foundColorType = colorTypes.find( ( colorType ) =>
					typeof colorType === 'string' ?
						colorType === name :
						Object.keys( colorType ).some( ( key ) => key === name )
				);
				const colorContextName =
					typeof foundColorType === 'string' ?
						foundColorType :
						Object.entries( foundColorType ).find( ( entry ) => entry[ 0 ] === name )[ 1 ];

				colorClass = `has-${ kebabCase(
					foundColorPaletteObject.slug
				) }-${ kebabCase( colorContextName ) }`;
			}

			return {
				...foundColorPaletteObject,
				class: colorClass,
			};
		},

		utilsToBind: { getMostReadableColor },
	} );
	attributePicker.colorUtils = attributePicker.utils;
	delete attributePicker.utils;

	return attributePicker;
}

export default function useColors( colorTypes ) {
	const colorPalette = useSelect(
		( select ) => select( 'core/block-editor' ).getSettings().colors || [],
		[]
	);

	return useCustomColors( colorTypes, colorPalette );
}

with-colors.js

export default function withColors( ...colorTypes ) {
	return createHigherOrderComponentWithMergeProps(
		() => useColors( colorTypes ),
		'withColors'
	);
}

In @wordpress/compose:

It adds createHigherOrderComponentWithMergeProps (shown above) for making it easier to make higher order components from hooks. This will become a more and more common pattern as we replace the implementation of more and more HOCs with their hooks counterparts.

It also adds the useCustomCompareDep, useShallowCompareDep, and useDeepCompareDep hooks for making it easier to use non-primitives in hooks dependency arrays. This was needed for useAttributePicker.

E.g.

useMemo( expensiveComputation, [ useDeepCompareDep( deeplyMutableObject ) ] );

Updates

How has this been tested?

All of the tests suites succeeded, but new unit tests have to be written for the new functions once consensus on the APIs is reached.

Types of Changes

New Features

  • Add the useAttributePicker hook to abstract away common block code and use it to refactor the color and font size higher order components. They are now written with the new exported hooks, useColors, useCustomColors, and useFontSizes.
  • Add createHigherOrderComponentWithMergeProps for making it easier to make higher order components from hooks.
  • Add the useCustomCompareDep, useShallowCompareDep, and useDeepCompareDep hooks for making it easier to use non-primitives in hooks dependency arrays.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

…deps.

- Add `createHigherOrderComponentWithMergeProps` for making it easier to make higher order components from hooks.
- Add the `useCustomCompareDep`, `useShallowCompareDep`, and `useDeepCompareDep` hooks for making it easier to use non-primitives in hooks dependency arrays.
…or HOCs.

- Added the `useAttributePicker` hook to abstract away common block code and used it to refactor the color higher order components. They are now written with the new exported hooks, `useColors` and `useCustomColors`.
@epiqueras epiqueras self-assigned this Jun 20, 2019
@epiqueras epiqueras added [Feature] Block API API that allows to express the block paradigm. [Feature] Extensibility The ability to extend blocks or the editing experience [Status] In Progress Tracking issues with work in progress labels Jun 20, 2019
@epiqueras epiqueras added this to the Future milestone Jun 20, 2019
@youknowriad youknowriad requested a review from mtias June 20, 2019 05:39
@epiqueras
Copy link
Contributor Author

Refactoring withFontSizes was even easier:

use-font-sizes.js

export default function useFontSizes( fontSizeNames ) {
	const fontSizes = useSelect(
		( select ) => select( 'core/block-editor' ).getSettings().fontSizes || [],
		[]
	);

	return useAttributePicker( {
		names: fontSizeNames,
		valuesEnum: fontSizes,

		findEnumValue: ( fontSizeObject, newFontSizeValue ) =>
			fontSizeObject.size === newFontSizeValue,
		mapEnumValue: ( fontSizeObject ) => fontSizeObject.slug,

		mapAttribute: ( fontSizeSlugOrCustomValue ) => {
			const foundFontSizeObject = fontSizes.find(
				( fontSizeObject ) => fontSizeObject.slug === fontSizeSlugOrCustomValue
			) || { size: fontSizeSlugOrCustomValue };

			return {
				...foundFontSizeObject,
				class:
					foundFontSizeObject.slug &&
					`has-${ kebabCase( foundFontSizeObject.slug ) }-font-size`,
			};
		},
	} );
}

with-font-sizes.js

export default function withFontSizes( ...fontSizeNames ) {
	return createHigherOrderComponentWithMergeProps(
		() => useFontSizes( fontSizeNames ),
		'withFontSizes'
	);
}

Relevant commit:

5726843 Block Editor: Add useFontSizes and use it to refactor withFontSizes.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having some hard time reviewing this PR because it's big and because it's not clear how it achieves the goal.

Questions on my mind:

  • I have a hard time understanding how useAttributePicker works. What would be the minimal example of usage this hook? Is it just a low-level hook never meant to be used directly? In that case, is it a necessary abstraction or just a way to factorize code?

  • How does withColors defer from useColor? Can we have an example of a block refactored to use useColor? I think at the time of writing, they have the exact same functionality but I was wondering if we can push the abstraction further. How can we absorb more boilerplate? Say I have a block and I want to support block level colors. Should I have a hook that also returns the "element" to add to the block inspector (To clarify, I'm not sure it's a good idea but that's the kind of question I'd appreciate to see explored, the more we simplify for blocks to support "common features", the better is)?

@youknowriad
Copy link
Contributor

Also, how can we split this PR into smaller PRs to ease reviews?

@mcsf
Copy link
Contributor

mcsf commented Jul 1, 2019

Thanks for taking this on. I had a line of thought similar to Riad's, so I'll wait for some answers there before resuming the review.

@epiqueras
Copy link
Contributor Author

I have a hard time understanding how useAttributePicker works. What would be the minimal example of usage this hook? Is it just a low-level hook never meant to be used directly? In that case, is it a necessary abstraction or just a way to factorize code?

It's the result of consolidating the logic that was in with-colors and with-font-sizes. A simple use case would look something like this:

const MyAlignableComponent = () => {
	const attributePicker = useAttributePicker( {
		names: [ 'topAlignment', 'bottomAlignment' ],
		valuesEnum: [ 'left', 'right' ],

		mapAttribute: ( alignment ) => `has-align-${ alignment }`, // has-align-left || has-align-right
	} );
	return (
		<>
			<div className={ attributePicker.topAlignment }>Top</div>
			<button onClick={ () => attributePicker.setTopAlignment( 'left' ) }>
				Left
			</button>
			<button onClick={ () => attributePicker.setTopAlignment( 'right' ) }>
				Right
			</button>

			<div className={ attributePicker.bottomAlignment }>Bottom</div>
			<button onClick={ () => attributePicker.setBottomAlignment( 'left' ) }>
				Left
			</button>
			<button onClick={ () => attributePicker.setBottomAlignment( 'right' ) }>
				Right
			</button>
		</>
	);
};

How does withColors defer from useColor? Can we have an example of a block refactored to use useColor? I think at the time of writing, they have the exact same functionality but I was wondering if we can push the abstraction further. How can we absorb more boilerplate? Say I have a block and I want to support block level colors. Should I have a hook that also returns the "element" to add to the block inspector...

Yeah, apart from the fact that one is a HOC and one is a hook, the API is the same.

We could make useColors, by default, but optionally disabled, automatically insert PanelColorSettings into the block inspector or add new entries to its colorSettings prop if already present (that way you can call it more than once and with different color names). How does that sound to you?

There are surely lots of other block feature helpers I can come up with. I'll be mostly thinking about FSE for the moment, but some ideas that come to mind are: form <-> attribute binding, responsive helpers, pagination for huge attributes, tools for notifications and tooltips, hooks for any stateful WordPress API.

Also, how can we split this PR into smaller PRs to ease reviews?

We can have a PR for each of the main hooks added here:

  • useAttributePicker
  • useColors
  • useFontSizes

Should I break this into 3 PRs and add the PanelColorSettings injection to the useColors one before we move forward?

@mcsf
Copy link
Contributor

mcsf commented Jul 2, 2019

How does withColors defer from useColor? Can we have an example of a block refactored to use useColor? I think at the time of writing, they have the exact same functionality but I was wondering if we can push the abstraction further. How can we absorb more boilerplate? Say I have a block and I want to support block level colors. Should I have a hook that also returns the "element" to add to the block inspector...

Yeah, apart from the fact that one is a HOC and one is a hook, the API is the same.

Having myself a propensity to perhaps too eagerly generalise, I think we could approach this problem depth-first rather than breadth-first. What I mean by this is that, as Riad suggests, we should isolate the first use case—say, colors—and see how far we can go with a single dedicated hook for it. That entails studying current uses of withColors: as mentioned, there are potential reductions of boilerplate, and maybe an overall improvement of developer experience if we can provide clearer interfaces than what we currently have.

Otherwise, we run the risk of merely porting one kind of enhancer (HoC) to the other (hook) and, with it, any usage issues.

It's the result of consolidating the logic that was in with-colors and with-font-sizes.

Reiterating: by going deep with colours, then moving to fonts, we can build a better picture of how these should relate. To be clear: if in the end it turns out that we just power both hooks with a useAttributePicker, then that's fine. But I wouldn't place a quasi-DSL ahead of our yet-to-determine needs. (I'm a sucker for good DSLs, but something as specific as an attribute-picking middleware I'd optimise for a flow that uses or resembles standard language constructs.)


We could make useColors, by default, but optionally disabled, automatically insert PanelColorSettings into the block inspector or add new entries to its colorSettings prop if already present (that way you can call it more than once and with different color names). How does that sound to you?

I think this is a very good experiment!

Should I break this into 3 PRs and add the PanelColorSettings injection to the useColors one before we move forward?

Based on my previous feedback, I'd start with a PR just for colours, then work on fonts in an orthogonal PR, and then iterate in a PR dependent on the former two.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments related to the workaround colors. I guess it is a nice idea to separate this PR in multiple PR's. I think it would also be nice to not focus on the current API. It was built in a time where the UI was different and we did not had the number of different use cases we have now so it was expanded but always taking back-compatibility in consideration.
I think if we go for a hooks implementation we don't need to follow the current API, feel free to try a different one.
The objective seems to be to reduce the amount of code a block needs to implement color functionality. I thought about that problem and it seems a possibility is for the hook or HOC to return a react element with a PanelColorSettings component with all the props already passed to it, or to add context communication between useColors/withColors and PanelColorSettings. If the user just did the component would retrieve things from the context that were set by useColors/withColors, a context approach would also allow the contrast checking functionality to use it. These are just some ideas I have, feel free to follow a different path.

*
* @param {string[]} attributePickerOptions.names The list of attributes to manage.
*
* @param {*[]} attributePickerOptions.valuesEnum The list of values attributes can be set to.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valuesEnum may make it look like we are referring to a pure enumeration e.g: one may expect it to be ['red', 'blue'] while its form is an array of objects [{ name: 'Red', slug: 'red', color: '#ff0000' },{ name: 'Blue', slug: 'blue', color: '#0000ff' }]. Maybe we can call it valueSet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can also have an ENUM of objects. Here is a good analogy for the difference between the two:

https://stackoverflow.com/a/32017860

) || { color: colorSlugOrCustomValue };

let colorClass;
if ( foundColorPaletteObject.slug ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function seems complex mainly because of retriving the colorContextName. Would something like this work?

const foundColorType = find( colorTypes, ( colorType ) => !! colorType && !! colorType[ name ] );
const colorContextName = foundColorType[ name ] || name;

Given that the mapping may execute frequently we may also consider computing an object based on colorTypes whose keys are the name and whose value is the color context or even the class directly. The names passed to useAttributePicker would then be just the keys of that object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah something in that style does read way better.

Given that the mapping may execute frequently we may also consider computing an object based on colorTypes whose keys are the name and whose value is the color context or even the class directly. The names passed to useAttributePicker would then be just the keys of that object.

Also might be a good idea.

* @param {*[]} [attributePickerOptions.mapAttributeDeps] List of items that, if they fail a shallow equality test, will
* cause attributes to be mapped again. Useful if your `mapAttribute` has closures.
*
* @param {{ string: Function }} [attributePickerOptions.utilsToBind] Object with functions to bind to `valuesEnum` and merge into the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relationship between utilsToBind and the other properties is not obvious, also the fact that we always bind valuesEnum seems really restrict, I may want to find to the selected value for example.
It's not hard for someone using this hook to create util functions and bind them to properties of the returned object (useMemo call where the returned object is a dependency, and return a new object that uses the returned object and adds the functions with binds to properties returned from useAttributePicker). Maybe we can remove utilsToBind to simplify this component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought binding to valuesEnum could be common after seeing the current colors implementation. But I wasn't fully convinced by this API either. It feels like it's "forcing" the generalization.

@epiqueras
Copy link
Contributor Author

Thanks for all the feedback everyone! 😄

I'll close this in favor of the new approaches proposed.

@epiqueras epiqueras closed this Jul 3, 2019
@youknowriad youknowriad deleted the try/refactoring-common-block-code branch July 4, 2019 09:21
@mtias
Copy link
Member

mtias commented Jul 4, 2019

I agree with @mcsf that we should focus on how can we streamline the use of specific block tools (like colors) so that the developer experience is tangibly better. That includes less boilerplate, less opportunities for messing up, easier to maintain, and also more flexible to tweak if needed.

One example of the latter is the Alignments toolbar — we've wanted to group them all in a dropdown (instead of being a row in the toolbar) for certain blocks, and that is not trivial to do in the current setup without affecting all usages.

@epiqueras epiqueras removed the [Status] In Progress Tracking issues with work in progress label Jul 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Extensibility The ability to extend blocks or the editing experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants