Style: The trailing parentheses placement on the previous line makes it difficult to discern this as the second argument of withSelect or the second entry in the array.

I'd suggest either:

withSelect( ( select ) => {
	const settings = select( 'core/editor' ).getEditorSettings();
	return {
		colors: get( settings, [ 'colors' ], DEFAULT_COLORS ),
	};
} ),

Or:

withSelect(
	( select ) => {
		const settings = select( 'core/editor' ).getEditorSettings();
		return {
			colors: get( settings, [ 'colors' ], DEFAULT_COLORS ),
		};
	}
),

Unrelated: I don't think this should be capitalized as though it were a component, since it's not. Confused me thinking we had incorrect usage. Something with a with prefix would be good to reflect it being a higher-order component (admitting there is a bit of confusion on higher-order-components vs. higher-order-component-creators like withFallbackStyles).

It seems withFallbackStyles is a better name I will rename it in a separate PR.

Filed #7355 so we don't forget (plus it's a good first bug).

We don't need compose?

export default withColors( 'backgroundColor', { textColor: 'color' } )( ButtonEdit );

Can we define somewhere what we mean by "color context" in practice? Each time I look at this, I forget what it's for. I see it's primarily used as part of the generated class name modifier e.g. has-green-color ?

I extended the comment to describe the color context.

Rather than copying setAttributes from props into state, could instead we assign the prop callback as an instance-bound function?

Similar to what we do in withDispatch:

Getting a general sense we might be trying to hard to optimize this with caching copies of relevantAttributes, colors, setAttributes. Maybe we could compare against prevState.mergeProps to see if the attribute value has changed, rather than doing the shallow equality before.

Something like:

return class extends Component {
	constructor( props ) {
		super( props );
		this.state = {};
	}

	static getDerivedStateFromProps( { attributes, colors, setAttributes }, prevState ) {
		return reduce( colorMap, ( mergePropsAcc, colorContext, colorAttributeName ) => {
			const ufColorAttrName = upperFirst( colorAttributeName );
			const customColorAttributeName = `custom${ ufColorAttrName }`;
			const colorName = attributes[ colorAttributeName ];
			const customColor = attributes[ customColorAttributeName ];
			const colorValue = getColorValue( colors, colorName, customColor );
			if ( get( mergePropsAcc, [ colorAttributeName, 'value' ] ) === colorValue ) {
				return mergePropsAcc;
			}

			mergePropsAcc[ colorAttributeName ] = {
				name: colorName,
				class: getColorClass( colorContext, colorName ),
				value: colorValue,
			};
			mergePropsAcc[ `set${ ufColorAttrName }` ] =
				setColorValue( colors, colorAttributeName, customColorAttributeName, setAttributes );
			return mergePropsAcc;
		}, prevState );
	}

	render() {
		return (
			<WrappedComponent
				{ ...{
					...this.props,
					colors: undefined,
					...this.state,
				} }
			/>
		);
	}
};

Hi @aduth, I changed the code in the direction of your feedback.

There's a tab here after the space following return. Not sure why ESLint didn't pick it up.

Nice catch I made my editor show tabs and spaces all the time, to avoid this problem in the future :)

Is bind valid for an arrow function? Is it needed?

No needed, arrow functions cannot be rebinded and the bind for this is automatic. It was corrected 👍

For what case do we need && prevColorValue ? Can we have a unit test?

It is a trivial case and the condition is executed all the time, without && prevColorValue during the first execution, both prevColorValue and colorValue are undefined. So we would always execute newState[ colorAttributeName ] = prevColorObject; and never compute the object. I will add this as a comment to make clear why && prevColorValue is needed.

I also updated the code to use && prevColorObject, this makes the condition more clear.

There shouldn't be a comma after "though".

Haha, I've never heard of kebab case 😄 🍖

Why is this import { default as withColorsDeprecated } from './with-colors-deprecated'; instead of just import withColorsDeprecated from './with-colors-deprecated';?

ufColorAttrName is a lot of abbreviation–I'm not sure what it means and I tried to read above but it was hard to tell from context.

What does Acc stand for? Sorry, a lot of these abbreviations are totally new to me and might throw other new contributors off.

Accumulator, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/reduce 😃

Ah, it would be nice to expand the variable name then, that is a lot more instructive 🤷‍♂️

Also noting that we have a horrible inconsistency with our naming of the Array#reduce accumulator variable throughout the codebase, often also referred to as memo or result.

Should we follow Mozilla docs?

I filed #7378 for it; setting it to accumulator everywhere works for me; we could do something like settersAccumulator here and elsewhere if we want more clarity and I think that'd be fine.

I also think we have space for previous instead of prev 😉

What is the && here for?

It would be nicer to use double-quotes here rather than escaping the single-quotes inside. Is that against the style rules? (If so: ouch. 😅)

Obj strikes again 😉

Filed #7355 so we don't forget (plus it's a good first bug).