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

[jesstelford]

Builds on functionality introduced in #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]

It's hard to tell what changes to some of our utilities are necessary here and I'm afraid the scss changes only confuse things more, and also make it harder to migrate away from scss or to a post css processor.

Can you outline why these changes were made and why you couldn't fix the bug(s) by using existing utilities? I'm talking specifically about the changes in _responsive-props.scss, css.ts

[kyledurand]

Since we're only using Entries from type-fest in this one spot, and we know the shape, and we're casting, I think we can do away with the type-fest import and type this manually.

[jesstelford]

I really don't want to maintain the Entries type - it's complicated enough that I barely understand it, but I do know it does exactly what I needed, and I landed on this solution after attempting to manually type it first (though admittedly I quickly rage quit because I'd had enough of TypeScript's cryptic errors and nonesense inability to understand basic things like JS's Object.entries).

[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]

I think this is really close. My main concern now is that when passing in a non-responsive value it doesn't override the responsive defaults which will lead to confusion.

Secondary concern is that adding to the mixin complexity will add tech debt when trying to remove sass. Do you have a plan for that? I think we might be removing sass sooner rather than later and not sure post css conversion will work 1:1. This is not a blocker

[kyledurand]

Can we override these defaults when only a single value gets passed in?

[jesstelford]

Ooh, good catch - I missed that case. I'll patch that up this morning 👍

add tech debt when trying to remove sass.

Depends on what we're aiming for;

1. If we're going to PostCSS, then most of the functionality can be moved into postcss-mixins by the looks of it.
2. If we're going to CSS-in-JS, then the functionality can be converted to JS fairly easily.
3. If we're going to pure CSS without PostCSS plugins or CSS-in-JS then we could either fallback to creating and injecting stylesheets, or reassess the overall strategy.

[jesstelford]

Was able to fix the defaults issue with @gwyneplaine's help 🎉

I amended an earlier commit, so I've put the diff below for you to review.

tl;dr: Reverted back to using initial for scoping the CSS custom properties, then moved the default into the media queries (where they should have been all along). The new find-default-with-fallback() function matches the semantics of the consumer-facing responsive props API. ie; things like $default: ('sm': 4, 'lg': 8) are effectively the same as: $default: ('xs': null, 'sm': 4, 'md': 4, 'lg': 8, 'xl': 8).

diff --git a/polaris-react/src/styles/shared/_responsive-props.scss b/polaris-react/src/styles/shared/_responsive-props.scss
index 319a70f2a..1ae4fc569 100644
--- a/polaris-react/src/styles/shared/_responsive-props.scss
+++ b/polaris-react/src/styles/shared/_responsive-props.scss
@@ -6,11 +6,37 @@
   @return type-of($var) == 'null';
 }
 
+// NOTE: Order is important here; will search backwards through this list to
+// find fallback values
+$breakpointKeys: ('xs', 'sm', 'md', 'lg', 'xl');
+
+@function find-default-with-fallback($default, $breakpoint) {
+  $initialIndex: index($breakpointKeys, $breakpoint);
+
+  // If the given '$breakpoint' doesn't exist, just return null
+  @if is-null($initialIndex) {
+    @return null;
+  }
+
+  // Search backwards until a non-null value is found in the $defaults map
+  // NOTE: Arrays in SASS are 1-based, so we're iterating down to AND INCLUDING
+  // index "1" (the first element).
+  @for $index from $initialIndex through 1 {
+    $value: map-get($default, nth($breakpointKeys, $index));
+    @if not is-null($value) {
+      @return $value;
+    }
+  }
+
+  // Couldn't find a non-null default, so return null
+  @return null;
+}
+
 @function get-default($default: null, $key: null) {
   $value: null;
   @if is-map($default) {
     @if not is-null($key) {
-      $value: map-get($default, $key);
+      $value: find-default-with-fallback($default, $key);
     }
   } @else {
     $value: $default;
@@ -21,18 +47,17 @@
 @mixin scope-custom-property(
   $componentName,
   $componentProp,
-  $responsively: false,
-  $default: null
+  $responsively: false
 ) {
   @if $responsively {
     // stylelint-disable -- Polaris component custom properties
-    --pc-#{$componentName}-#{$componentProp}-xs: #{get-default($default, 'xs')};
-    --pc-#{$componentName}-#{$componentProp}-sm: #{get-default($default, 'sm')};
-    --pc-#{$componentName}-#{$componentProp}-md: #{get-default($default, 'md')};
-    --pc-#{$componentName}-#{$componentProp}-lg: #{get-default($default, 'lg')};
-    --pc-#{$componentName}-#{$componentProp}-xl: #{get-default($default, 'xl')};
+    --pc-#{$componentName}-#{$componentProp}-xs: initial;
+    --pc-#{$componentName}-#{$componentProp}-sm: initial;
+    --pc-#{$componentName}-#{$componentProp}-md: initial;
+    --pc-#{$componentName}-#{$componentProp}-lg: initial;
+    --pc-#{$componentName}-#{$componentProp}-xl: initial;
   } @else {
-    --pc-#{$componentName}-#{$componentProp}: #{get-default($default)};
+    --pc-#{$componentName}-#{$componentProp}: initial;
   }
 }
 
@@ -42,14 +67,12 @@
   $declarationProp,
   $default: null
 ) {
-  @include scope-custom-property(
-    $componentName,
-    $componentProp,
-    false,
-    $default
-  );
+  @include scope-custom-property($componentName, $componentProp, false);
 
-  #{$declarationProp}: var(--pc-#{$componentName}-#{$componentProp});
+  #{$declarationProp}: var(
+    --pc-#{$componentName}-#{$componentProp},
+    #{get-default($default)}
+  );
 }
 
 @mixin responsive-props(
@@ -58,19 +81,20 @@
   $declarationProp,
   $default: null
 ) {
-  @include scope-custom-property(
-    $componentName,
-    $componentProp,
-    true,
-    $default
-  );
+  @include scope-custom-property($componentName, $componentProp, true);
 
-  #{$declarationProp}: var(--pc-#{$componentName}-#{$componentProp}-xs);
+  #{$declarationProp}: var(
+    --pc-#{$componentName}-#{$componentProp}-xs,
+    #{get-default($default, 'xs')}
+  );
 
   @media #{$p-breakpoints-sm-up} {
     #{$declarationProp}: var(
       --pc-#{$componentName}-#{$componentProp}-sm,
-      var(--pc-#{$componentName}-#{$componentProp}-xs)
+      var(
+        --pc-#{$componentName}-#{$componentProp}-xs,
+        #{get-default($default, 'sm')}
+      )
     );
   }
 
@@ -79,7 +103,10 @@
       --pc-#{$componentName}-#{$componentProp}-md,
       var(
         --pc-#{$componentName}-#{$componentProp}-sm,
-        var(--pc-#{$componentName}-#{$componentProp}-xs)
+        var(
+          --pc-#{$componentName}-#{$componentProp}-xs,
+          #{get-default($default, 'md')}
+        )
       )
     );
   }
@@ -91,7 +118,10 @@
         --pc-#{$componentName}-#{$componentProp}-md,
         var(
           --pc-#{$componentName}-#{$componentProp}-sm,
-          var(--pc-#{$componentName}-#{$componentProp}-xs)
+          var(
+            --pc-#{$componentName}-#{$componentProp}-xs,
+            #{get-default($default, 'lg')}
+          )
         )
       )
     );
@@ -106,7 +136,10 @@
           --pc-#{$componentName}-#{$componentProp}-md,
           var(
             --pc-#{$componentName}-#{$componentProp}-sm,
-            var(--pc-#{$componentName}-#{$componentProp}-xs)
+            var(
+              --pc-#{$componentName}-#{$componentProp}-xs,
+              #{get-default($default, 'xl')}
+            )
           )
         )
       )

[gwyneplaine]

@kyledurand here's a local reproduction url with the above cases in playroom.

[kyledurand]

/snapit

[gwyneplaine]

(unofficially) lgtm ✅

[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]

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]

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.