[material-ui] Remove display="block" usage to work with Pigment CSS

[siriwatknp]

Issue

There are some components that are using deprecated system props, display="block".

This produces incorrect behavior when using Pigment CSS. Below is an example of rendering <CardHeader title="…" subheader="…">:

Emotion:

Pigment CSS

Solution

The best solution that I found so far is to use CSS variables (the same approach I used with Joy UI).

The Typography component has a display of var(--Typography-display) so that other components can control the display without creating higher CSS specificity.

---

• I have followed (at least) the PR section of the contributing guide.

[mui-bot]

Netlify deploy preview

https://deploy-preview-43307--material-ui.netlify.app/

FormControlLabel: parsed: -3.75% 😍, gzip: -3.12% 😍
@material-ui/core: parsed: +0.03% , gzip: +0.11%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 43c3ae1

[mnajdova]

There are some components that are using deprecated system props, display="block".

In order to avoid a bigger breaking change, can we just update the prop to have some of the valid CSS values and support it:

display: 'block' | 'inline-block' | 'none' | 'flex' | 'grid'

And document that if people used the breakpoints syntax only in those cases they should use the CSS var approach. What do you think about it?

I mean it has a discrete set of values, we could still support at least these values.

[aarongarciah]

I'm not sure about having a CSS var as part of the component API. Looks like introducing a new approach that's not followed in other parts of Material UI. I think we should think about this a bit more before proceeding. What other alternatives do we have?

Can we recommend using the sx prop to override the display property? It'd support responsive syntax, too, right? i.e. do what @mnajdova says (supporting a regular display prop with a discrete set of values) and point users to sx in case they need responsive syntax.

[siriwatknp]

How about adding <Typography block />? @mnajdova @aarongarciah

I don't think we should add all possible values for the display. There are more than 10 values according to https://developer.mozilla.org/en-US/docs/Web/CSS/display.

Using this approach creates too many classes that are likely not used.

[aarongarciah]

I think a block prop is not a good API. If we later want to support another display value we end up with additional props. If we expose an API to customize the display property, I think it should cover at least the most common/expected display values in typography elements.

If we introduce a breaking change (removing the display prop), I'd prefer to point users to use the sx prop, which supports any display value. It's probably the most coherent decision because we don't offer the display in other components (afaik), not sure why Typography should be different since we're moving away from system props. Probably because is more common to change the display property in typography elements?

Otherwise, I'd suggest doing display="block | inline | inline-block | flex | ..." but using a locally scoped CSS var (not part of the public API) and declare it inline in the component, if the intention is to minimize the generated CSS classes. Pseudo-code:

const TypographyRoot = styled('span', {
  display: var(--_Typography-display)
});

const Typography = (props) => {
  return <TypographyRoot style={{ "--_Typography-display": props.display }}>;
};

// <Typography display="inline-flex" />

If it's acceptable to remove the display prop, suggesting the sx prop might be the best choice we have today (including a codemod?).

[siriwatknp]

I found a way to fix it without introducing any CSS var or props, using :where is possible and the specificity is still (0, 1, 0), so using sx can override the display too.

What do you think about 43c3ae1 @aarongarciah @mnajdova ?

[aarongarciah]

@siriwatknp I'm assuming Emotion users can keep using <Typography display="block"> (even if deprecated) and Pigment CSS users need to use <Typography sx={{ display: 'block' }}>. I think that's a good trade-off.

[siriwatknp]

Why not using sx prop?

Because we don't have a way to make it work with both Emotion and Pigment CSS. Let's say we use sx prop, this is how it looks like:

// CardHeader.js
<Typography
-  display="block"
    {…titleTypographyProps}
+  sx={[
+     { display: 'block' },
+       …Array.isArray(titleTypographyProps?.sx) ? titleTypographyProps?.sx : [titleTypographyProps?.sx]
+  ]}
>

The above code works with Emotion, but not with Pigment CSS (yet).