Add support for non-responsive values to `Grid`'s `gap`, `columns`, and `areas` props.

[jesstelford]

Builds on functionality introduced in https://github.com/Shopify/polaris/pull/10404.

In service of bringing Grid's responsive props in line with the rest of the system, I also standardized some other parts of the system along the way.

I purposely kept these as individual commits for easier reviewing and also so they retain their history, meaning we should do fast-forward merge instead of a merge commit ("Rebase and merge" in GitHub's UI) so the commits don't get squashed.

See the commits tab for the changes.

[kyledurand]

Just noticed you want to merge this into the next branch. Shouldn't bug fixes / minor changes go into main?

We'll also want to /snapit and compare admin areas where Grid is used

[kyledurand]

We need to ship this or a simplified version

[jesstelford]

Shouldn't bug fixes / minor changes go into main?

Yeah, but in this case I wanted to align with the changes in next without introducing a whole bunch of merge conflicts, and figured since we're rolling out v12 real soon anyway, it makes it easier to skip main.

[jesstelford]

It's hard to tell what changes to some of our utilities are necessary here

Fair enough, there's a lot going on in this PR. The commits were split up to try make it clear which changes are related to altering the utilities and for what purpose. But to expand on each of them:

1. Support default CSS variables beyond 'initial'

   • We're leveraging CSS Vars property of being DOM-inherited instead of Cascade-inherited (ie; it will crawl DOM nodes looking for CSS custom properties, instead of crawling the CSS cascade)
   • The CSS var() lookup will stop whenever any value is found. initial is a valid value, so the lookup algorithm stops.
   • But when initial is used in a var, it will cause that property to not be applied to the final calculated CSS.
   • With that in place, we've got a (basic) emulation of CSS scope while it's still being spec'd.
   • There are cases where we want the component to have a valid "default" value for some properties. There are two ways to do this:
     1. --some-var: initial; --some-var: <default>; Where we always set initial to enforce the "scope", then manually set the "" value after, or;
     2. --some-var: <default>; Where we just set the value to the default at the start.
   • At first it doesn't seem like much of a difference until you consider setting different defaults for each breakpoint. Option 1 is then duplicating the custom property for every breakpoint which is wasteful and confusing when inspecting the DOM. However, Option 2 is much clearer and has less duplication.
   • Since we're already configuring responsive props with a single mixin call, it makes sense to also configure the default values at the same time.
   • Later, this is used by Grid which has different defaults per breakpoint in a simple $default: ('xs': 6, 'lg': 12)

2. Standardize how we map over responsive props within components

   • This is just removing duplicate code and cleaning up the function signature in the process

3. Tighten up the types and deduplicate CSS Var generation code

   • I wish this commit didn't have to exist, but since we're using TS, there were a handful of cases where it was either not enforcing types tightly enough, or was giving erroneous errors when used by Grid.
   • Basically it's a TS bugfix commit.

4. Add support for non-responsive values to Grid's gap, columns, and areas props

   • This was the ultimate goal of these changes, and fixes the linked issue.
   • It greatly simplifies the SASS of Grid, standardises it along side all our other components, and leverages the previous 3 commits to do so.

the scss changes only confuse things more

Interesting! I find the new format of the SCSS file to be much easier to read and understand with much reduced duplication and chance for human error to sneak in. It also standardizes it with all our other components, so ideally once you've groked one, you've groked them all.

also make it harder to migrate away from scss or to a post css processor.

I'd argue the abstraction makes it easier; Instead of having to manually migrate each instance where we have multiple breakpoints with initials being set, etc, we've now got a single mixin which needs to be changed to whatever a new system might need. For example, switching to Vanilla Extract would mean simply rewriting the responsive-props mixin in JS, and shift the callsites to the .tsx files instead of .scss files instead of having to copy/paste huge chunks of arbitrary media queries, etc.

[jesstelford]

We'll also want to /snapit and compare admin areas where Grid is used

I'll definitely need help with this one - I often struggle to get spin to do what I need / find those places of usage actually in the Admin.

[kyledurand]

/snapit

[jesstelford]

@kyledurand of note in this PR is a bug fix for the differing behaviour of columns, areas, and gap props on Grid to make their responsiveness align correctly with the semantics of our responsive props across Polaris. It's to do with the way we fallback to lower and lower breakpoint values if the current breakpoint doesn't have an explicit value.

columns and areas

Current behaviour:

1. If set, use an explicit value passed for the current breakpoint. Else, go to Step 2.
2. If set, use a default value for the current breakpoint. Else, go to Step 3.
3. If not at at xs breakpoint, go down one breakpoint and go to Step 1. Else, go to Step 4.
4. Set the value to initial, resulting in not applying the property.

New behaviour:

1. If set, use an explicit value passed for the current breakpoint. Else, go to Step 2.
2. If not at at xs breakpoint, go down one breakpoint and go to Step 1. Else, go to Step 3.
3. If set, use a default value for the current breakpoint. Else, go to Step 4.
4. Set the value to initial, resulting in not applying the property.

Another way to think about it is how the props are expanded to have concrete values for each breakpoint:

For example, given a default of ('xs': 6, 'lg': 12), the prop values expand as such:

columns=	Current behaviour	New behaviour
3	n/a	{xs: 3, sm: 3, md: 3, lg: 3, xl: 3}
{md: 3}	{xs: 6, sm: 6, md: 3, lg: 12, xl: 12}	{xs: 6, sm: 6, md: 3, lg: 3, xl: 3}
{md: 3, lg: 2}	{xs: 6, sm: 6, md: 3, lg: 2, xl: 2}	{xs: 6, sm: 6, md: 3, lg: 2, xl: 2}
{md: 3, xl: 1}	{xs: 6, sm: 6, md: 3, lg: 12, xl: 1}	{xs: 6, sm: 6, md: 3, lg: 3, xl: 1}

gap

Before this PR, gap was implemented differently to columns and areas:

Current behaviour:

1. If set, use an explicit value passed for the current breakpoint. Else, go to Step 2.
2. Set the value to the default.

New behaviour: (same as for columns and areas)

1. If set, use an explicit value passed for the current breakpoint. Else, go to Step 2.
2. If not at at xs breakpoint, go down one breakpoint and go to Step 1. Else, go to Step 3.
3. If set, use a default value for the current breakpoint. Else, go to Step 4.
4. Set the value to initial, resulting in not applying the property.

For example, given the default of 4, the prop values expand as such:

gap=	Current behaviour	New behaviour
3	n/a	{xs: 3, sm: 3, md: 3, lg: 3, xl: 3}
{md: 3}	{xs: 4, sm: 4, md: 3, lg: 4, xl: 4}	{xs: 4, sm: 4, md: 3, lg: 3, xl: 3}
{md: 3, lg: 2}	{xs: 4, sm: 4, md: 3, lg: 2, xl: 4}	{xs: 4, sm: 4, md: 3, lg: 2, xl: 2}
{md: 3, xl: 1}	{xs: 4, sm: 4, md: 3, lg: 4, xl: 1}	{xs: 4, sm: 4, md: 3, lg: 3, xl: 1}

[kyledurand]

/snapit

[kyledurand]

Thanks for the breakdown @jesstelford I'm pretty good with the changes, but we need to snap this and make sure it at least passes web ci before merging. Just trying to sort that out now

[jesstelford]

Looks like the snapshots are failing to publish, but I'm not sure why... Is it because the base is next? How're we doing snapshots on other branches into next?

[jesstelford]

@laurkim noticed an issue with color being defaulted incorrectly:

Noticing some regressions with text when running locally though where the color isn't being set properly.

I've tracked it down to the difference between initial and inherit for color. This would have been an existing bug in our system, except for a compounding bug which cancelled it out:

When a color prop isn't passed, the CSS custom property's value is set to undefined, which means it's not set on the style attribute, so the declaration of color: var(--pc-box-color) becomes invalid, and falls back to the user agent's default of color: inherit.

The new behaviour in this PR enforces consistency in CSS custom properties, and set the default to initial always. While this was technically the most correct approach for scoping our variables (ie; if you want nested text to have the same color, you'd have to specify it explicitly), it doesn't match our consumer's expectation of how CSS works: It is expected that some properties will inherit from their parent automatically (eg; color, font, etc). So, I've fixed that by setting the default to inherit for the properties which need it based on data from https://www.npmjs.com/package/@webref/css.

Here's the diff of changes made to ensure some declarations are inherited.

---

An alternative implementation might be to mirror the existing behaviour of not passing the custom property through to the style attribute when a value isn't set.

If we went down this path, we'd need to move the "default" values up into the JS, then pass those down to the media queries via style tags. Unfortunately that would mean we'd be passing those defaults down for every instance of a component instead of just once per definition of a component. For example, Box has a default border-width: 0. If the page renders 45 <Box>es, there would potentially be 45 instances of style="--pc-box-border-width-default: 0" as opposed to just setting --pc-box-border-width: 0 once in the generated Box.css file.

[jesstelford]

🤔 I can't figure out what's causing these "19 changes" in Chromatic...

The screenshots seem to indicate that vertical padding is different, but when I load up the snapshots (base vs this PR), there's no difference, and I can't replicate it locally.

I'm wondering if it's a timing issue of some kind with client side rendering 🤔 🤷

[github-actions[bot]]

Hi! We noticed there hasn’t been activity on this PR in a while. After 30 days, it will close automatically.

If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed.

[translation-platform[bot]]

Localization quality issues found

The following issues may affect the quality of localized translations if they are not addressed:

• The value Clear for key Polaris.ActionList.SearchField.clearButtonLabel is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
• The value Search for key Polaris.ActionList.SearchField.search is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
• The value Search actions for key Polaris.ActionList.SearchField.placeholder is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.

Please look out for other instances of this issue in your PR and fix them as well if possible.

Questions about these messages? Hop in the #help-localization Slack channel.