Ensure non-responsive React props are properly encapsulated when converted to CSS Custom Properties

[jesstelford]

Fixed #10393

[kyledurand]

This is looking good. As you can probably tell I'm not a fan of mixins. Not a blocker. If you do go the mixin route can you add a comment about why it's needed and where it should be used?

[kyledurand]

My take here is to remove this mixin and include @include scope-custom-property where you need it. In the future we'll have atomic css from our tokens and shouldn't need all these custom properties everywhere. This added mixin complicates that clean up work.

The previous css was also much easier to read

[jesstelford]

Agreed this is obfuscating some things which I also don't like, but it is enforcing a basic level of standardization which is kind of the reason this bug appeared in the first place (lack of standardization).

My approach here was to follow the pattern established by @mixin responsive-props(). The motivation being to avoid this issue coming back up again in the future (I can imagine future-me seeing --pc-foo-whatever: initial and thinking "That's completely useless, let's be a good engineer and remove it" without realizing it's important. Similarly for naming of breakpoint props - I actually want that encapsulated away somewhere (mixin or function, I don't mind) so it can be consistent with little effort from the dev (again, to avoid future accidental errors being introduced).

Maybe I'm being overly cautious, but the fact this bug existed for so long without anyone noticing implies to me if we accidentally reintroduce it somewhere, it'll go unnoticed again for a while.

I initial thought we could use a @function instead, and keep the CSS declaration in the callsite which would be easier to understand, but I don't think that's possible with the way we're doing the media query stuff unfortunately. Perhaps a PostCSS plugin might be a better solution for both of these cases? Keep the input CSS clean, but output fully responsive token sets transparently (kind of like autoprefixer, but more autoresponsivizer? 😛)

In the future we'll have atomic css from our tokens

Does that involve switching to a CSS-in-JS solution? Because without it, we'll still need to do this "donut scope" encapsulation of the CSS variables step regardless of what they're named.

[kyledurand]

Does that involve switching to a CSS-in-JS solution

Yes and no, I guess. I'm thinking we need css classes created from tokens, so we'd apply a class (based off of a prop) for padding for example and would automatically be scoped. This would clean up a ton of our css and remove a lot of buggy custom properties

[kyledurand]

We should never have nested ShadowBevels right? What's this for? Can you /snapit and test the admin thoroughly if you think we need this added scope?

[jesstelford]

Because <ShadowBevel> accepts children, it's possible for them to be nested (even if it may not make sense from a UX perspective).

It also comes baked into <Card>, but that hard-codes zIndex="32" so wont fall afoul of the inheritance bug (the bug only occurs when there's no explicit value set - when not set, nested <ShadowBevels> don't pass style={{ '--pc-shadow-bevel-z-index': zIndex }}, so it will incorrectly inherit their z-index from a parent <ShadowBevel>).

The final value gets applied to a ::before pseudo element located within the <ShadowBevel>, which itself sets z-index: 0 to create a new stacking context, so I think this could only introduce a regression in one case: Where there's an outer <ShadowBevel> with a high zIndex prop set (say; 1000), containing a nested <ShadowBevel> without a zIndex prop passed (so it incorrectly inherits the zIndex: 1000 prop), and there happens to be a child of the nested <ShadowBevel> (within the same stacking context) with an explicit z-index set which is lower than the zIndex prop passed to the outer <ShadowBevel>...

A quick Grokt shows no usages of <ShadowBevel> outside the Polaris repo, so the above situation doesn't occur afaict 🎉

Happy to test this more thoroughly, but can you think of a way to narrow down the scope of what to test besides attempting to load every page that uses a <Card>? 😅

[kyledurand]

We don't export the ShadowBevel component. It's internal only