[Emotion] Convert global `className` utilities #6124

cee-chen · 2022-08-11T00:18:35Z

Summary

I strongly recommend following along by commit, as I did a few extra side cleanup items (missing logical properties, mixin API updates, documentation) alongside the global conversions.

Checklist

Checked in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Added documentation
Added or updated jest and cypress tests
Checked for breaking changes and labeled appropriately (this PR deliberately has no breaking changes / does not (yet) remove the IE utility - will be addressing that in a follow-up PR)
A changelog entry exists and is marked appropriately

- [ ] Props have proper autodocs and playground toggles
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
~~- [ ] Updated the Figma library counterpart~~

+ fix missing CSS logical property on text truncation mixin + convert textLeft/Right to logical CSS - note that we can't use the logicalTextAlign util for this as !important can't be passed to that util

cee-chen · 2022-08-11T01:19:02Z

src/global_styling/utility/utility.tsx

+    .eui-isFocusable:focus {
+      // Forcing focus ring on non-EUI elements
+      ${euiFocusRing(euiThemeContext)}
+    }


Question: is this global utility still valid? I'm not seeing any uses either in Kibana or EUI, and I feel like our *:focus reset should basically already covers this:

eui/src/global_styling/reset/global_styles.tsx

Lines 87 to 89 in 6d390a2

*:focus {

${euiFocusRing(euiThemeContext)}

}

Thoughts?

It seems that it was being used in an old version of EUI: https://github.com/lcawl/eui/blob/8073430a578673eb9944c20cc39ef0752c9aadc4/src-docs/src/views/guidelines/colors/core_palette.js#L51.

I couldn't find any uses in Kibana and in recent versions of EUI. So I would say it's safe to delete.

Fantastic, I'll go ahead and delete it. Thanks @miukimiu!!

edit: 626c66f

kibanamachine · 2022-08-11T01:43:26Z

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

@mixin

+ convert euiFullHeight Sass mixin to Emotion + move @mixin euiFullHeight to `mixins/_helpers` instead of living in `_utility`

- since that's where the className was documented

- this requires adding a new `logicalCSSWithFallback` util, since overflow-inline/block is not yet supported on non-FF browsers + fix incorrect overflow mapping - inline should be x and block should be y

+ convert mixin to accept the entire euiThemeContext instead of just the euiTheme to match all other mixins

kibanamachine · 2022-08-11T02:30:57Z

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

kibanamachine · 2022-08-11T17:07:16Z

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

cee-chen · 2022-08-15T15:48:24Z

Hey @breehall! If you have a time this week, do you mind helping code review this PR? Thank you!

miukimiu

Thanks for converting these global className utilities. LGTM! 🎉

miukimiu · 2022-08-16T15:17:00Z

src/components/breadcrumbs/breadcrumb.styles.ts

@@ -81,10 +81,10 @@ export const euiBreadcrumbContentStyles = (euiThemeContext: UseEuiTheme) => {
    // Types
    page: css`
      &:is(a):focus {
-        ${euiFocusRing(euiTheme, 'inset')};
+        ${euiFocusRing(euiThemeContext, 'inset')};


Much better! 🥳

miukimiu · 2022-08-16T15:18:10Z

src/components/toast/global_toast_list.styles.ts

@@ -34,8 +35,7 @@ export const euiGlobalToastListStyles = (euiThemeContext: UseEuiTheme) => {
      ${logicalCSS('bottom', 0)};
      ${logicalCSS('width', `${euiToastWidth + euiTheme.base * 5}px`)}; /* 2 */
      ${logicalCSS('max-height', '100vh')}; /* 1 */
-      overflow-y: auto; // Fallback for the 'overflow-inline' logical property, which is not yet supported
-      ${logicalCSS('overflow-y', 'auto')};
+      ${logicalCSSWithFallback('overflow-y', 'auto')};


That's a great idea. 🥳

miukimiu · 2022-08-16T15:34:56Z

src-docs/src/views/scroll/scroll_x.tsx

@@ -84,7 +84,7 @@ export default () => {
              <p>
                To mask the top and bottom of the scrolled content, indicating
                visually that there is more content below, pass in true to the
-                second paremeter <EuiCode>mask</EuiCode>.
+                second parameter <EuiCode>mask</EuiCode>.
              </p>
              <p>
                <EuiCode>{"useEuiOverflowScroll('x', true);"}</EuiCode>


Something that we need to decide as a team. We normally document the hooks instead of the function versions. But I use much more functions than hooks. Why? Because in component.styles.ts we can't use hooks.

If we search for useEuiShadow we find 0 usages in EUI. But euiShadow is being used multiple times. So I wonder if we should be documenting both functions and hooks or prefer documenting the functions rather than hooks.

Our docs are consumer facing, so we document the hooks because we expect consumers to be using hooks more than passing our euiThemeContext around. Internally in EUI we pass our theme around a lot, but consumers are much less likely to do so, so they'll probably just want a hook that handles getting the theme for them.

TBH, this is kind of an architectural choice we made too when setting up our Emotion styles pattern. We could have decided to make all our styles files/functions hooks instead, e.g. useEuiComponentStyles() which then allows us to call hook utils of all our styles instead of having to pass euiTheme around manually. But then hooks can't be called conditionally or in callbacks, so there's pros/cons to everything.

cc @thompsongl for more thoughts

miukimiu · 2022-08-16T15:47:18Z

src/global_styling/utility/utility.tsx

+      ${logicalCSS('width', '100% !important')}
+    }
+    .eui-fullHeight {
+      ${euiFullHeight()}


This is something that I never know... When we're calling functions should we end the line with semicolons?

Most of our functions are already fully fledged CSS line(s) and contain their own ending semicolon, so I remove the extra semicolon after use because it's unnecessary (although Emotion/CSS is smart enough to ignore extra semicolons).

A few functions do not have the trailing semicolon (although that's not common) so those will need it. Essentially It depends on the function; you may have to right click / trace the function to check its output.

+ already being handled by global reset

kibanamachine · 2022-08-16T16:31:50Z

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

cee-chen · 2022-08-16T16:35:39Z

@breehall Sorry for the whiplash, I'm going to go ahead and merge this PR after CI passes since there's another PR that relies on it (#6125). Please feel free to review if you want - I can address any questions/feedback in a follow-up PR!

cee-chen changed the title ~~[Emotion] Convert global className utils~~ [Emotion] Convert global className utilities Aug 11, 2022

cee-chen added 2 commits August 10, 2022 17:20

Convert text globals/utilities to Emotion

c88f1ea

+ fix missing CSS logical property on text truncation mixin + convert textLeft/Right to logical CSS - note that we can't use the logicalTextAlign util for this as !important can't be passed to that util

Pass euiTheme to globalStyles and convert euiNumberFormat

28d626b

cee-chen force-pushed the global-utils branch 3 times, most recently from a2c7201 to 2b68854 Compare August 11, 2022 01:13

cee-chen marked this pull request as ready for review August 11, 2022 01:17

cee-chen requested a review from miukimiu August 11, 2022 01:17

cee-chen commented Aug 11, 2022

View reviewed changes

cee-chen added 2 commits August 10, 2022 19:00

Convert display utils to Emotion

b1d25fa

+ convert euiFullHeight Sass mixin to Emotion + move @mixin euiFullHeight to `mixins/_helpers` instead of living in `_utility`

Document euiFullHeight under scroll utils

5c2d5ef

- since that's where the className was documented

cee-chen force-pushed the global-utils branch from 2b68854 to c821825 Compare August 11, 2022 02:00

cee-chen added 3 commits August 10, 2022 19:03

Convert scroll globals to Emotion (+ add logical CSS properties)

48fe87f

- this requires adding a new `logicalCSSWithFallback` util, since overflow-inline/block is not yet supported on non-FF browsers + fix incorrect overflow mapping - inline should be x and block should be y

Convert .eui-isFocusable global to Emotion

a557e69

+ convert mixin to accept the entire euiThemeContext instead of just the euiTheme to match all other mixins

Convert responsive globals to Emotion

3f4187e

cee-chen force-pushed the global-utils branch from c821825 to 9d510b6 Compare August 11, 2022 02:03

cee-chen mentioned this pull request Aug 11, 2022

[Emotion] Add eslint rule for CSS logical properties #6125

Merged

2 tasks

cee-chen added 2 commits August 11, 2022 09:39

Add snapshot test for quick checking of global styles output

ab32f6f

Changelog

3e288c0

cee-chen force-pushed the global-utils branch from 9d510b6 to 3e288c0 Compare August 11, 2022 16:39

cee-chen requested a review from breehall August 15, 2022 15:48

miukimiu approved these changes Aug 16, 2022

View reviewed changes

Remove .eui-isFocusable - not being used anywhere

626c66f

+ already being handled by global reset

cee-chen enabled auto-merge (squash) August 16, 2022 16:40

cee-chen merged commit 6d91a09 into elastic:main Aug 16, 2022

cee-chen deleted the global-utils branch August 16, 2022 16:57

cee-chen mentioned this pull request Aug 22, 2022

Remove IE CSS utilities, mixins, and non-flex/sizing fallbacks #6154

Merged

12 tasks

1Copenut mentioned this pull request Sep 8, 2022

Bumping EUI to version 64.0.4 elastic/kibana#140323

Merged

cee-chen mentioned this pull request Sep 21, 2022

[Meta][CSS-in-JS] Component conversions #5685

Closed

49 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Emotion] Convert global `className` utilities #6124

[Emotion] Convert global `className` utilities #6124

cee-chen commented Aug 11, 2022 •

edited

Loading

cee-chen Aug 11, 2022 •

edited

Loading

miukimiu Aug 16, 2022

cee-chen Aug 16, 2022 •

edited

Loading

kibanamachine commented Aug 11, 2022

kibanamachine commented Aug 11, 2022

kibanamachine commented Aug 11, 2022

cee-chen commented Aug 15, 2022

miukimiu left a comment

miukimiu Aug 16, 2022

miukimiu Aug 16, 2022

miukimiu Aug 16, 2022 •

edited

Loading

cee-chen Aug 16, 2022 •

edited

Loading

miukimiu Aug 16, 2022

cee-chen Aug 16, 2022 •

edited

Loading

kibanamachine commented Aug 16, 2022

cee-chen commented Aug 16, 2022

[Emotion] Convert global className utilities #6124

[Emotion] Convert global className utilities #6124

Conversation

cee-chen commented Aug 11, 2022 • edited Loading

Summary

Checklist

cee-chen Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

miukimiu Aug 16, 2022

Choose a reason for hiding this comment

cee-chen Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Aug 11, 2022

kibanamachine commented Aug 11, 2022

kibanamachine commented Aug 11, 2022

cee-chen commented Aug 15, 2022

miukimiu left a comment

Choose a reason for hiding this comment

miukimiu Aug 16, 2022

Choose a reason for hiding this comment

miukimiu Aug 16, 2022

Choose a reason for hiding this comment

miukimiu Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

cee-chen Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

miukimiu Aug 16, 2022

Choose a reason for hiding this comment

cee-chen Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Aug 16, 2022

cee-chen commented Aug 16, 2022

[Emotion] Convert global `className` utilities #6124

[Emotion] Convert global `className` utilities #6124

cee-chen commented Aug 11, 2022 •

edited

Loading

cee-chen Aug 11, 2022 •

edited

Loading

cee-chen Aug 16, 2022 •

edited

Loading

miukimiu Aug 16, 2022 •

edited

Loading

cee-chen Aug 16, 2022 •

edited

Loading

cee-chen Aug 16, 2022 •

edited

Loading