[CSS-in-JS] Add `logicalShorthandCSS` util + lint rule #6429

cee-chen · 2022-11-22T20:55:03Z

Summary

Unfortunately, shorthand properties that describe boxes such as margin and padding do not currently automatically respond to logical changes in display direction (see w3c/csswg-drafts#1282).

Until an official spec is created by W3C, this utility is a stopgap measure to convert (e.g.) margin shorthands to margin-inline and margin-block respectively.

Example usage within Emotion (intentionally similar in usage to logicalCSS()):

margin: 0 ${euiTheme.size.s};

becomes

${logicalShorthandCSS('margin', `0 ${euiTheme.size.s}`)}

I've also added a lint rule for this, which flags shorthand usage with multiple values. Single values (same value applied to all 4 sides) do not need to be converted to a logical property, and both the lint rule and utility intelligently detect this.

Note: there is no --fix behavior currently as I deemed it too unnecessarily complex to try and automate a fix - devs will have to manually import and call logicalShorthandCSS. Should W3C implement a single-line shorthand keyword or property, this will become much easier to do at that point, so I would strongly suggest waiting for that.

I strongly recommend following along by commit with whitespace changes disabled.

QA

Checked the following components for CSS output and visual smoke test:

General checklist

Added or updated jest and cypress tests
A changelog entry exists and is marked appropriately

cee-chen · 2022-11-22T21:08:39Z

scripts/eslint-plugin/css_logical_properties.js

+// Construct a regex to find shorthands with more than one value
+// Shorthands with only 1 value set (applied to all 4 sides) are fine
+const logicalShorthandsRegexes = _shorthands.map((property) => {
+  const start = `${property}:\\s*`; // e.g. `margin: `
+  const interpolatedVar = '\\${[^}]+}'; // e.g. `${euiTheme.something}`
+  const staticValue = '[^\\s(!};]'; // e.g. `20px` - any value that doesn't have a space, !keyword, } interpolation, or semicolon
+  const value = `(${interpolatedVar}|${staticValue})+`; // Values can be variable or static
+  const end = ';'; // Ensure that the match ends with a semicolon
+
+  return `(${start}(${value}\\s+)+${value}${end})`; // Only match multiple values (spaces between values)
+});
+const logicalShorthandsRegex = logicalShorthandsRegexes.join('|');


Just want to mention a couple things: I'm aware this is a moderately insane regex, so apologies for that. I tried to make it easier to read by splitting up the various parts but I'm not 100% sure I succeeded 🥲

I will say breaking it up like this does make it a little difficult to easily copy/paste into (e.g.) an online regex tester. For future me's records, my recommendation is doing this:

Add console.log(logicalShorthandsRegex); below line 27

Run yarn jest scripts/eslint-plugin/css_logical_properties.test.js and copy the console log output

Paste just one property regex into your regex tool (rather than all joined regexes), like so:

Debug/play around with the various portions accordingly to try and solve whatever false positive or negative may be occurring

Update Jest unit tests after to ensure said false positive/negative is or isn't caught by the linter

cee-chen · 2022-11-22T21:22:39Z

scripts/eslint-plugin/css_logical_properties.js

-        const templateContents = node.quasis || [];
-        templateContents.forEach((quasi) => {
-          const stringLiteral = quasi?.value?.raw;
-          if (!stringLiteral) return;
+        const stringLiteral = context.getSourceCode().getText(node);
+        const content = stringLiteral.replace(/^`|`$/g, ''); // Strip starting/ending backtick


Just want to quickly highlight why this change to context.getSourceCode().getText(node) (docs) was necessary:

If getText() isn't used, AST unfortunately breaks up node.quasis between each ${interpolatedVar} usage, meaning a basic padding: 0 ${euiTheme.size.s}; line isn't correctly caught/linted.

Here's an example of how AST breaks down template literal quasis which you can see yourself via AST Explorer:

For the purposes of our lint rule for EUI, since we don't necessarily care what the var interpolates to[1], just that we want to check if there's more than one var, so getting the flat/non-interpolated text is the solution. It also incidentally saves us from another forEach iteration so is possibly more performant in any case.

[1] This is because our vars are generally pretty safe... although if someone does something like const foo = '0 1 2 3'; padding: ${foo}; our linter would not catch this 🤔 Hopefully we just don't do that kind of shenanigans though.

kibanamachine · 2022-11-22T21:23:33Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6429/

kibanamachine · 2022-11-22T21:53:07Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6429/

cee-chen · 2022-11-23T23:31:55Z

While QAing the updated components, I realized that my logical CSS properties for border-color/width/style were not quite correct. Also, border-radius is a ludicrously special case due to the fact that it addresses corners, not edges (and thus has no -inline/-block properties), and additionally has slash syntax. So I did what any insane person would do and wrote way too much overengineered code to handle an edge case we don't actually have in EUI because I was having fun. Please do not judge me.

- which automatically handles converting CSS shorthands for 4 box sides to 2 separate logical properties - see codeblock comment for more info - a single property/keyword does not yet exist, but when one is added the CSS spec we can flip this util over

- single values are ok; no need to make them logical if they're the same on all sides - no `fix` option for this sadly, since the extra logic would be overly complex (either have to handle newlines/indentations, or have to handle a util import, both of which feel overly complex) - note: `context.getSourceCode().getText(node)` is needed here over iterating through quasi's, because AST breaks up the template literal by vars found in within the literal, whereas we really only want to evaluate the exact text of variable usage itself - this actually also saves us from another iteration forEach so is unintentionally a slight perf boost(?)

…to the single util

- requires a slightly different syntax from margin/padding/inset

me: i hate math also me: i'm gonna spend several hours doing what basically amounts to light geometry and matrices

- noticed while grepping for shorthand properties

- was triggering lint rule

+ fix other example title

kibanamachine · 2022-11-24T01:54:05Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6429/

breehall

Thank you for adding the background into this PR. Out of curiosity, was this broken and we didn't realize it?

I pulled the branch and played around the the regex that was created in an online editor. The regex is crazy, but your solution is very elegant!

With the introduction of logicalBorderRadiusCSS, should we add some documentation to the Borders page like we did with Size?

breehall · 2022-11-30T20:40:08Z

src/global_styling/functions/logical_shorthands.ts

+  switch (values.length) {
+    case 1:
+      // If it's the same value all around, no need to use logical properties
+      return `${property}: ${value};`;
+    case 2:
+      verticalBlockValue = values[0];
+      horizontalInlineValue = values[1];
+      break;
+    case 3:
+      verticalBlockValue = `${values[0]} ${values[2]}`;
+      horizontalInlineValue = values[1];
+      break;
+    case 4:
+    default:
+      verticalBlockValue = `${values[0]} ${values[2]}`;
+      horizontalInlineValue = `${values[3]} ${values[1]}`; // Note: left (4th value) comes before right (2nd value)
+      break;
+  }


Love this. It feels very self-explanatory when reading.

Woohoo, very satisfying to know! Also just to quickly address one of your top level q's in this thread:

Out of curiosity, was this broken and we didn't realize it?

Our logical CSS work has been more of a speculative enhancement. There isn't (yet) a specific need to support direction or writing-mode in Kibana or Elastic, but since it's easier for EUI to set the standard and other apps and teams follow, we're kind of making it up as we go.

With the introduction of logicalBorderRadiusCSS, should we add some documentation to the Borders page like we did with Size?

The logicalShorthandCSS actually supports border radius (just without the ignoreZeroes) flag, so I didn't know if I needed extra documentation for this - I mostly wanted the ignoreZeroes option for our own EUI css where we know we're not overriding existing border-radius 🤔

I'll think about this more and circle back on it! To be honest, I'm hoping we can eventually remove that special logic totally if W3 comes out with a more handy CSS spec where the API remains the same except for a keyword.

breehall · 2022-11-30T20:57:26Z

scripts/eslint-plugin/css_logical_properties.js

+const logicalShorthandsRegexes = _shorthands.map((property) => {
+  const start = `${property}:\\s*`; // e.g. `margin: `
+  const interpolatedVar = '\\${[^}]+}'; // e.g. `${euiTheme.something}`
+  const staticValue = '[^\\s(!};]'; // e.g. `20px` - any value that doesn't have a space, !keyword, } interpolation, or semicolon
+  const value = `(${interpolatedVar}|${staticValue})+`; // Values can be variable or static
+  const end = ';'; // Ensure that the match ends with a semicolon
+
+  return `(${start}(${value}\\s+)+${value}${end})`; // Only match multiple values (spaces between values)
+});


🧠 Played around with the margin and scroll-margin regex patterns in an online playground and tried to come up with various patterns. From what I tested, the regex seems solid to me!

Woohoo! I'm sure it's not bulletproof, but it should hopefully capture 90%+ of common use cases, and we can continue to tweak (or line-disable if needed) it as we go on.

[email protected] ⏩ [email protected] --- ## [`71.0.0`](https://github.com/elastic/eui/tree/v71.0.0) - Implemented new `EuiRange` and `EuiDualRange` designs where the `levels` are now on top of the tracks ([#6092](elastic/eui#6092)) - Added `discuss` and `dotInCircle` glyphs to `EuiIcon` ([#6434](elastic/eui#6434)) - Added `article` glyph to `EuiIcon` ([#6437](elastic/eui#6437)) - Changed the `EuiProvider` usage warnings to not rely on development mode. ([#6451](elastic/eui#6451)) **Breaking changes** - `EuiDualRange` now explicitly requires both `min` and `max` via props types, to match `EuiRange` ([#6092](elastic/eui#6092)) - `EuiRange` and `EuiDualRange`'s `compressed` size no longer impacts track or level sizes, but continues to compress tick and input sizes. ([#6092](elastic/eui#6092)) - Removed all variables for the following components from EUI's theme JSON files: ([#6443](elastic/eui#6443)) - `euiCollapsibleNav*` - `euiColorPicker*` - `euiContextMenu*` - `euiControlBar*` - `euiDataGrid* `(except for z-indices and cell padding sizes) - `euiDatePicker*` - `euiSuperDatePicker*` - `euiDragAndDrop*` - `euiEuiEmptyPrompt*` - `euiFilePicker*` - `euiRange*` - `euiHeaderLinks*` - `euiKeyPad*` - `euiMarkdownEditor*` - `euiResizable*` - `euiSelectable*` - `euiSideNav*` - `euiStep*` - `euiSuggest*` - `euiTable*` (except for color variables) - `euiTooltip*` - `euiButtonFontWeight`, `euiButtonDefaultTransparency`, and `euiButtonMinWidth` - If you were importing any of the above removed JSON variables, we strongly recommend using generic color or sizing variables from `useEuiTheme()` instead. ([#6443](elastic/eui#6443)) **CSS-in-JS conversions** - Converted `EuiRange` and `EuiDualRange` to Emotion; Removed `$euiRangeThumbRadius` ([#6092](elastic/eui#6092)) - Added a new `logicalStyles` utility that automatically converts all non-logical properties in a `style` object to their corresponding logical properties ([#6426](elastic/eui#6426)) - Added a new `logicalShorthandCSS` utility that automatically converts `margin`, `padding`, and other 4-sided shorthands to their corresponding logical properties ([#6429](elastic/eui#6429)) - Added a new `logicalBorderRadiusCSS` utility that automatically converts `border-radius` to corresponding logical properties ([#6429](elastic/eui#6429)) Co-authored-by: Constance Chen <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Stratoula Kalafateli <[email protected]>

cee-chen added the emotion label Nov 22, 2022

cee-chen commented Nov 22, 2022

View reviewed changes

cee-chen force-pushed the logicals branch 2 times, most recently from 929b747 to 7611e97 Compare November 23, 2022 23:07

cee-chen added 11 commits November 23, 2022 17:26

Add logicalShorthandCSS util

eb3681f

- which automatically handles converting CSS shorthands for 4 box sides to 2 separate logical properties - see codeblock comment for more info - a single property/keyword does not yet exist, but when one is added the CSS spec we can flip this util over

[EuiModal] Example of two separate inline/block properties converted …

0c388dc

…to the single util

[lint] Fix all instances of multi-value shorthands to new util

2615685

Handle border side shorthand properties

7ef5cba

- requires a slightly different syntax from margin/padding/inset

[Misc fix] catch multi-space edge cases

1f5c613

Add ridiculously over-engineered logic for border-radius util

411ae33

me: i hate math also me: i'm gonna spend several hours doing what basically amounts to light geometry and matrices

[misc cleanup] swap unnecessary calc()s to static mathWithUnits

32528f8

- noticed while grepping for shorthand properties

[misc docs] fix incorrect property

d917440

- was triggering lint rule

changelog

034fc8d

[Docs] Add utility example

c06efa8

+ fix other example title

cee-chen force-pushed the logicals branch from fd8e82c to c06efa8 Compare November 24, 2022 01:26

miukimiu requested review from miukimiu and removed request for miukimiu November 28, 2022 13:41

breehall approved these changes Nov 30, 2022

View reviewed changes

cee-chen merged commit 0c76929 into elastic:main Nov 30, 2022

cee-chen deleted the logicals branch November 30, 2022 22:49

breehall mentioned this pull request Dec 6, 2022

Bumping EUI to 71.0.0 elastic/kibana#147142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CSS-in-JS] Add `logicalShorthandCSS` util + lint rule #6429

[CSS-in-JS] Add `logicalShorthandCSS` util + lint rule #6429

cee-chen commented Nov 22, 2022 •

edited

Loading

cee-chen Nov 22, 2022 •

edited

Loading

cee-chen Nov 22, 2022 •

edited

Loading

kibanamachine commented Nov 22, 2022

kibanamachine commented Nov 22, 2022

cee-chen commented Nov 23, 2022

kibanamachine commented Nov 24, 2022

breehall left a comment

breehall Nov 30, 2022

cee-chen Nov 30, 2022 •

edited

Loading

breehall Nov 30, 2022

cee-chen Nov 30, 2022

[CSS-in-JS] Add logicalShorthandCSS util + lint rule #6429

[CSS-in-JS] Add logicalShorthandCSS util + lint rule #6429

Conversation

cee-chen commented Nov 22, 2022 • edited Loading

Summary

QA

General checklist

cee-chen Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

cee-chen Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Nov 22, 2022

kibanamachine commented Nov 22, 2022

cee-chen commented Nov 23, 2022

kibanamachine commented Nov 24, 2022

breehall left a comment

Choose a reason for hiding this comment

breehall Nov 30, 2022

Choose a reason for hiding this comment

cee-chen Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

breehall Nov 30, 2022

Choose a reason for hiding this comment

cee-chen Nov 30, 2022

Choose a reason for hiding this comment

[CSS-in-JS] Add `logicalShorthandCSS` util + lint rule #6429

[CSS-in-JS] Add `logicalShorthandCSS` util + lint rule #6429

cee-chen commented Nov 22, 2022 •

edited

Loading

cee-chen Nov 22, 2022 •

edited

Loading

cee-chen Nov 22, 2022 •

edited

Loading

cee-chen Nov 30, 2022 •

edited

Loading