Will getCSSRules need to return React-safe style camel case props as well, e.g., { style: { paddingTop: '19px', ... } }

I'm seeing mixed output:

I know the focus is on rendering styles to the page, so apologies if I'm jumping the 🔫

Indeed, I guess I didn't pay enough attention here :).

For me getCSSRules was supposed to be an internal API (not exposed) but I had to find something to make the current style hook and theme.json style renderer continue to work with the padding moved to the style engine. So I just used what I had. I'll need to check where the mixing happens and just pick one for now.

Interesting problem here. I had a play with a couple of different approaches and settled on switching getCSSRules to return the camelCase keys (since it's an array of JS objects, it felt slightly more natural to me for it to be using the JS style attribute convention), and then get generate to be responsible for converting it to kebab-case when outputting the CSS styles. Updated in this commit: a756e44

That seemed to fix it for me, though it does introduce importing another couple of lodash functions. I imagine we might want to see if we can get rid of the lodash dependency if we're expecting the style engine to also be run in standalone frontend environments outside of Gutenberg, but for the moment at least, it made it quicker to get the fix in.

Happy to change the behaviour here if folks have a preference for the shape of the attributes!

Ideally at the end of the style engine implementation, this "constant" might be removed entirely if we did our work properly :)

I think we should probably ignore the properties with useEngine: true here kind of like https://github.com/WordPress/gutenberg/pull/37978/files#diff-292053c9ec98ebdd73943767587e8093315ad3a2bf01ca3b5352efc48326becdR89

Good catch! I've added useEngine to the check that bails early in the loop in 31981a1 — I've also updated the output slightly to match the change to getCSSRules discussed here.

Have you considered using the React.CSSProperties types for most types in this file?

e.g

type Props = {
  backgroundColor: CSSProperties[ 'backgroundColor' ];
}

Good question, we can probably move most of them over to CSSProperties (e.g. fontWeight, fontStyle, etc). For the Box type, since it's used in both margin and padding, it might need to be a bit more flexible than targeting a single CSS property. What do you think would work for that type?

Mhh, not sure about that. The main thing is that the idea of Box (an object with top right bottom and left props) doesn't really translate to CSS.

If we need to maintain the structure of these types, we could still use CSSProperties where we're using string, and leave the rest unchanged:

export type Box = {
	top?: CSSProperties[ 'top' ];
	right?: CSSProperties[ 'right' ];
	bottom?: CSSProperties[ 'bottom' ];
	left?: CSSProperties[ 'left' ];
};

export interface Style {
	spacing?: {
		margin?: CSSProperties[ 'margin' ] | Box;
		padding?: CSSProperties[ 'padding' ] | Box;
	};
	// ...
};

WDYT?

Oh, I quite like that, and it lets us keep the Box abstraction. It isn't quite right because it's referring to different properties, but the underlying type should be much the same, and it still serves the purpose of better communicating the desired type. I've updated in f0cbeff, and we can continue to tweak the types in follow-ups, too 🙂

Thank you! I think it's already much better.

It isn't quite right because it's referring to different properties, but the underlying type should be much the same,

Out of curiosity, what did you mean with "referring to different properties" ?

what did you mean with "referring to different properties" ?

Apologies, I could have been clearer there! What I meant is that CSSProperties[ 'top' ] refers to the top property and not for example padding-top — but since they both accept pretty much the same actual values, it doesn't matter too much that the type isn't precise. I think CSSProperty[ 'top' ] is preferable to a more complex type like CSSProperty[ 'top' ] | CSSProperty[ 'paddingTop ' ] | CSSProperty[ 'marginTop' ] which could get a bit unwieldy, even if it's technically more accurate.

But, happy to explore in follow-ups, of course!

Proposed a potential follow-up in #38894