[Emotion] Convert Sass typography mixins to JS #5854

cee-chen · 2022-04-28T20:59:31Z

Summary

For QA, please see: https://eui.elastic.co/pr_5854/#/utilities/text

For code review, I recommend following along by commit.

What this PR does:

Converts all remaining Sass mixins in _typography.scss to JS
- In the case of the euiTextShift, this was moved to functions/typography.ts and not mixins/_typography.ts as it's mostly an internal-only util
Adds documentation snippets for both Emotion and Sass mixins for consumer usage (Sass mixins were previously not documented alongside the CSS classes)

What this PR does not do:

Convert any components currently using the Sass mixins
Delete the old Sass mixins - that will be done all at once at a later period

Checklist

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

Not sure if a changelog is needed here Sass mixins were not previously documented - thoughts?

+ add optional arg for max-width property

…her than `mixin` - per feedback from Caroline, this util is mostly used internally and does not need to be publicly exported/documented

- a la https://elastic.github.io/eui/#/utilities/scroll

…bottom - to group a class utility and its mixin together visually

cee-chen · 2022-04-28T21:05:33Z

src/components/title/title.styles.ts

@@ -64,7 +64,7 @@ export const useEuiTitle = (
 */
 export const euiTitleStyles = ({ euiTheme }: UseEuiTheme) => ({
  euiTitle: css`
-    ${euiTextBreakWord}
+    ${euiTextBreakWord()}


I ended up converting this into a function to match other mixin usages. From a JS perspective I think it makes 'mixin' usage a bit clearer

src-docs/src/views/text_utilities/wrapping.tsx

src-docs/src/views/utility_classes/utility_classes_example.js

kibanamachine · 2022-04-28T21:28:33Z

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

…style page later This reverts commit 9fe7b65.

kibanamachine · 2022-04-29T18:32:22Z

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

cchaos

Thanks for doing all these conversions and adding tests. Lots of comments, but mostly repetitive stuff around language and just a few comments about the mixins themselves.

src/global_styling/mixins/_typography.ts

src/global_styling/mixins/_typography.test.ts

src/global_styling/functions/typography.ts

src-docs/src/views/text_utilities/text_utilities_example.js

cchaos · 2022-04-29T21:08:22Z

src/global_styling/mixins/_typography.ts

+export const euiNumberFormat = () => `
+  font-feature-settings: 'calt' 1, 'kern' 1, 'liga' 1, 'tnum' 1;
+`;


Here is actually an opprtunity to merge global styles. We have euiTheme.font.featureSettings which is set to 'calt' 1, 'kern' 1, 'liga' 1. In sass, there wasn't a good way to merge these, but here we can make this a hook and append just the 'tnum' 1 portion.

Ah fantastic, I totally missed that variable!

65517bf

Making the mixin use euiTheme now requires euiTheme as an arg, so I ended up making a hook version of this and documenting the hook version instead. I'd be curious to hear your thoughts on whether documenting both hook and non hook mixins feels confusing.

src-docs/src/views/text_utilities/numbers.tsx

cchaos · 2022-04-29T21:13:14Z

src-docs/src/views/text_utilities/align.tsx

+      description={
+        <p>
+          Changes the element’s text alignment property to{' '}
+          <EuiCode language="sass">text-align: left;</EuiCode>


I just realized we need to be changing these all to logical properties. 😬

I would still keep the name of the thing as left and right, but the output should be logical.

As in I should be changing this in Sass itself? Could we maybe wait until we convert our Sass-only text utils to CSS-in-JS globals before doing this?

Sorry no, I didn't realize this PR didn't contain the JS version of the Sass utility. When we do convert the pure utility, we need to make sure output uses the logical properties. Then in this description, instead of explicitly saying what the output style is, we'd just say something like:

Suggested change

<EuiCode language="sass">text-align: left;</EuiCode>

be aligned to the left (starting) side of its container.

Ahh gotcha! Makes sense.

I made that documentation tweak while here since I figured it was easy enough - LMK if you'd rather revert/wait until we tackle Sass/Emotion globals b4d2c2d

cchaos

An optional idea.

src-docs/src/views/text_utilities/wrapping.tsx

+ add hook version, since this util now references euiTheme

cee-chen · 2022-05-02T17:07:11Z

@cchaos by the way, any thoughts on whether this PR needs a changelog? I'm unsure as Sass mixins were previously undocumented.

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

kibanamachine · 2022-05-02T18:00:18Z

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

cchaos

@constancecchen , Yes it needs a changelog stating these new additions. It doesn't need to be under the conversion header. Just something like

Add euiNumberFormat(), ..., ... text utilities.

kibanamachine · 2022-05-02T23:24:47Z

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

cchaos

Looks great to me!

cee-chen added 9 commits April 28, 2022 12:51

Add unit tests for euiFontSize mixin

c966bca

Convert euiTextBreakWord to function + add unit test

16c7195

[misc] fix downstream mounted snapshot by changing to render

db454d8

Convert euiTextTruncate mixin from Sass to CSS-in-JS

90fc6de

+ add optional arg for max-width property

Convert euiNumberFormat mixin from Sass to CSS-in-JS

c1a3273

Convert euiTextShift from Sass to CSS-in-JS + move to functions rat…

545eba1

…her than `mixin` - per feedback from Caroline, this util is mostly used internally and does not need to be publicly exported/documented

[Docs] Move text utilities to their own page

27ac2f3

- a la https://elastic.github.io/eui/#/utilities/scroll

[Docs] Split text utilities into sections + add mixin snippets

c4f1e5e

[Docs] Add prop to ThemeExample that allows stacking/removing margin-…

9fe7b65

…bottom - to group a class utility and its mixin together visually

cee-chen added the emotion label Apr 28, 2022

cee-chen requested a review from cchaos April 28, 2022 20:59

cee-chen marked this pull request as ready for review April 28, 2022 21:00

cee-chen commented Apr 28, 2022

View reviewed changes

[PR feedback] Remove margin-bottom/stacking behavior - Caroline will …

14c4604

…style page later This reverts commit 9fe7b65.

cchaos suggested changes Apr 29, 2022

View reviewed changes

cchaos reviewed May 2, 2022

View reviewed changes

src-docs/src/views/text_utilities/wrapping.tsx Outdated Show resolved Hide resolved

src-docs/src/views/text_utilities/wrapping.tsx Outdated Show resolved Hide resolved

cee-chen added 4 commits May 2, 2022 10:05

[PR feedback] Documentation copy + example fixes

ab3f6b1

[PR feedback] Comment copy, remove IE workaround

e257273

[PR feedback] DRY out unit measurement tests

a3cff1f

[PR feedback] Fix euiNumberFormat to use euiTheme.font.featureSettings

65517bf

+ add hook version, since this util now references euiTheme

cee-chen and others added 3 commits May 2, 2022 10:24

[PR feedback] Add callouts to CSS utilities docs page

ec27f55

[PR feedback] Tweak text align documentation

b4d2c2d

[PR feedback] Document euiTextTruncate parameters in title

b0467f8

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

cee-chen requested a review from cchaos May 2, 2022 17:30

cchaos reviewed May 2, 2022

View reviewed changes

Add changelog entry

e38cd3a

cee-chen force-pushed the emotion/text-utilities branch from 54e470d to e38cd3a Compare May 2, 2022 22:54

cee-chen requested a review from cchaos May 3, 2022 16:36

cchaos approved these changes May 3, 2022

View reviewed changes

cee-chen merged commit 0bbacf0 into elastic:main May 3, 2022

cee-chen deleted the emotion/text-utilities branch May 3, 2022 18:13

cchaos mentioned this pull request May 5, 2022

[Docs] Better sectioning for new utilities #5871

Merged

4 tasks

breehall mentioned this pull request May 16, 2022

Upgrade EUI to v58.0.0 elastic/kibana#132257

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Emotion] Convert Sass typography mixins to JS #5854

[Emotion] Convert Sass typography mixins to JS #5854

cee-chen commented Apr 28, 2022 •

edited

Loading

cee-chen Apr 28, 2022

kibanamachine commented Apr 28, 2022

kibanamachine commented Apr 29, 2022

cchaos left a comment

cchaos Apr 29, 2022

cee-chen May 2, 2022

cee-chen May 2, 2022

cchaos Apr 29, 2022

cchaos Apr 29, 2022

cee-chen May 2, 2022

cchaos May 2, 2022

cee-chen May 2, 2022

cee-chen May 2, 2022

cchaos left a comment

cee-chen commented May 2, 2022

kibanamachine commented May 2, 2022

cchaos left a comment •

edited

Loading

kibanamachine commented May 2, 2022

cchaos left a comment

	<EuiCode language="sass">text-align: left;</EuiCode>
	be aligned to the left (starting) side of its container.

[Emotion] Convert Sass typography mixins to JS #5854

[Emotion] Convert Sass typography mixins to JS #5854

Conversation

cee-chen commented Apr 28, 2022 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

kibanamachine commented Apr 28, 2022

kibanamachine commented Apr 29, 2022

cchaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos left a comment

Choose a reason for hiding this comment

cee-chen commented May 2, 2022

kibanamachine commented May 2, 2022

cchaos left a comment • edited Loading

Choose a reason for hiding this comment

kibanamachine commented May 2, 2022

cchaos left a comment

Choose a reason for hiding this comment

cee-chen commented Apr 28, 2022 •

edited

Loading

cchaos left a comment •

edited

Loading