[Emotion] Convert EuiText, EuiTextAlign, and EuiTextColor by cee-chen · Pull Request #5895 · elastic/eui

cee-chen · 2022-05-11T19:06:02Z

Summary

Woof, this ended up being much larger than I thought it would be 💦 I strongly recommend following along by commit, in the following order:

EuiText conversion: (involves adding a new customScale property to our typography utils and creating an optional obj of properties instead of passing 4+ args)
- 24e389a
- 6103653
- 61dc217
- acbe167
- 2d326aa
- ec05b2e
- 0568917
- f864454
- 189e369
Partial EuiMarkdownFormat conversion (the portions of the mixin that really belong to EuiMarkdownFormat and not EuiText):
- 09e072d
EuiTextAlign conversion: (also contains updates to the logicals utilities, LMK if we are OK with this, if not I can revert and just use border-inline-start-width directly)
- 8fd4e19
- 42692fa
EuiTextColor conversion: (I'm unsure whether this conversion requires using our makeHighContrastColor function or if that's already baked in)

Checklist

Anything in the backlog that could be a quick fix for that component? (specific requests made by @cchaos)
Convert component-specific Sass vars to exported JS versions
Convert side specific padding, margin, and position to -inline and -block (Logical properties)
Reduce specificity where possible (usually by reducing class name chaining)

~~- [ ] Convert global Sass vars to JS version like sizing~~
~~- [ ] Use gap property to add margin between items if using flex~~
~~- [ ] Can any still existing .js files be converted to TS?~~
~~- [ ] All animations or transitions are wrapped in euiCanAnimate~~

QA

Does the output CSS match the previous CSS / as expected (I checked each and every single CSS output and confirmed that they were the same as before, aside from line height changes that already existed in our new typography utils)
Does the rendered class read as expected

src/components/text/text.tsx

cee-chen · 2022-05-11T19:18:38Z

src/components/text/text_align.tsx

-  const classes = classNames(
-    'euiTextAlign',
-    alignmentToClassNameMap[textAlign],
-    className
-  );
+  const styles = euiTextAlignStyles();
+  const cssStyles = [styles.euiTextAlign, styles[textAlign]];

  return (
-    <div className={classes} {...rest}>


Similarly, I felt safe completely removing the CSS classNames for this component due to low Kibana usage, that can all be quickly and easily replaced with our CSS text utilities in any case:

src/components/text/text_color.tsx

src/global_styling/functions/logicals.ts

src/components/text/text_color.styles.ts

src/components/link/link.styles.ts

kibanamachine · 2022-05-11T19:41:34Z

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

cchaos

Some early feedback after a quick scan of the code except for the styles.

src-docs/src/views/theme/typography/_typography_js.tsx

src/global_styling/functions/logicals.ts

src/components/text/text_color.styles.ts

src/components/markdown_editor/markdown_format.styles.ts

src/components/text/text.tsx

src/global_styling/functions/typography.ts

src/components/empty_prompt/_empty_prompt.scss

kibanamachine · 2022-05-17T20:25:07Z

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

TODO: EuiCode variables were not converted, per direction from Caroline

- simplifies usage

+ convert optional params to an options object + clean up euiFontSize and euiTitle fns - remove `scale` fallback since the utils don't really make sense without that param, and remove rem fallbacks in place of just single options fallback

- since there isn't a .9 scale size, we should just use `em` to make it relative - since `.euiCode` also uses `.9em`, we can now remove the `:not()` selector

- attempting to account for `em` margins + add :not(:last-child) selector per Caroline's request

…file - might as well do it while I'm here and thinking about it + expanded our logicals helper to include all logical `border` properties, since the converted CSS uses this

+ switch to logical properties

- text alignment already has its own CSS utils that users can call directly (that we can also convert any Kibanau sage into), so I feel very safe making this change, and I'd rather people not continue to hook into these classNames

- since they're no longer setting CSS - we should convert Kibana usages to the component

cchaos · 2022-05-24T17:54:05Z

I'll do a final check, but I think it's important to also get an eng review.

kibanamachine · 2022-05-24T18:14:36Z

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

kibanamachine · 2022-05-24T21:57:50Z

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

cee-chen · 2022-05-24T23:52:46Z

but I think it's important to also get an eng review.

I don't especially want to throw a brand new engineer into a super large PR (2k~ lines and 70+ files) that's had a bunch of disparate changes and back and forth (i.e., no easy git commit history to follow). I strongly think this is a waste of their time and ours.

Can we limit the code review we're requesting to a specific set of files you feel concerned about in terms of code quality? For example, would you want them to examine only the text.tsx/text_color.tsx/text_align.tsx files and maybe the euiFontSize mixin/font changes and all corresponding tests?

cchaos · 2022-05-25T13:27:23Z

Yes exactly, I mean to look at the trickier bits of code like you mentioned, the tests, clones, and mixins.

cee-chen · 2022-05-25T17:03:45Z

Thanks for clarifying Caroline! @thompsongl, would you mind doing engineering review on only the following components/files:

markdown_editor
- markdown_format.styles.ts
  - tests / snapshots
text
- text.tsx
  - tests / snapshots
- text.styles.ts
  - tests / snapshots
- text_color.tsx
  - tests / snapshots
  - text_color.styles.ts
- text_align.tsx
  - tests / snapshots
  - text_align.styles.ts
global_styling
- functions/typography.ts
- mixins/_typography.ts
  - tests / snapshots
(optional) wiki

cee-chen · 2022-05-25T17:04:10Z

jenkins test this

kibanamachine · 2022-05-25T17:30:55Z

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

thompsongl

Code changes look good 👍

src/components/markdown_editor/markdown_format.styles.ts

kibanamachine · 2022-05-25T22:13:20Z

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

cchaos

LGTM! Wasn't that fun!?

upcoming_changelogs/5895.md

cchaos · 2022-05-26T14:50:36Z

Jenkins, test this

kibanamachine · 2022-05-26T15:17:13Z

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

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

kibanamachine · 2022-05-26T16:24:48Z

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

kibanamachine · 2022-05-26T19:54:57Z

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

cee-chen added the emotion label May 11, 2022

cee-chen requested a review from cchaos May 11, 2022 19:06

cee-chen commented May 11, 2022

View reviewed changes

src/global_styling/functions/logicals.ts Show resolved Hide resolved

src/components/text/text_color.styles.ts Show resolved Hide resolved

src/components/link/link.styles.ts Show resolved Hide resolved

cee-chen changed the title ~~[Emotion] Convert[EuiText, EuiTextAlign, and EuiTextColor~~ [Emotion] Convert EuiText, EuiTextAlign, and EuiTextColor May 11, 2022

cchaos added this to the Elastic Stack 8.4 milestone May 16, 2022

cchaos suggested changes May 16, 2022

View reviewed changes

cee-chen force-pushed the emotion/eui-text branch from 86ca02e to 36be668 Compare May 17, 2022 19:56

cee-chen requested a review from cchaos May 17, 2022 20:10

cee-chen added 19 commits May 17, 2022 14:05

Convert .euiText CSS to Emotion

019bbe2

TODO: EuiCode variables were not converted, per direction from Caroline

[misc] Fix EuiLink theme params to match other files

cad7f03

- simplifies usage

Convert constrainedWidth modifier CSS to Emotion

cfc84c3

Add customScale option to typography functions

dbce67a

+ convert optional params to an options object + clean up euiFontSize and euiTitle fns - remove `scale` fallback since the utils don't really make sense without that param, and remove rem fallbacks in place of just single options fallback

Add start of euiScaleText conversion + remove sizes class names

ec2cfd2

Convert more of euiScaleText, + add unit tests

32cce3e

Convert more of euiScaleText + simplify code scale

969b33b

- since there isn't a .9 scale size, we should just use `em` to make it relative - since `.euiCode` also uses `.9em`, we can now remove the `:not()` selector

Convert remaining euiScaleText headings logic/math to JS

2a5a25c

- attempting to account for `em` margins + add :not(:last-child) selector per Caroline's request

Updoot millions of downstream snapshots/tests

b515f3b

Convert remaining euiMarkDownFormat styling in _text.scss to its own …

b9e149d

…file - might as well do it while I'm here and thinking about it + expanded our logicals helper to include all logical `border` properties, since the converted CSS uses this

Remove _text.scss

2402762

Convert EuiTextAlign to Emotion

40bc9f6

+ switch to logical properties

[EuiTextAlign] Opinionated: Remove className entirely

7d7ef9f

- text alignment already has its own CSS utils that users can call directly (that we can also convert any Kibanau sage into), so I feel very safe making this change, and I'd rather people not continue to hook into these classNames

Convert EuiTextColor to Emotion

7b6281d

[EuiTextColor] Opinionated: Remove classNames entirely

d9d7546

- since they're no longer setting CSS - we should convert Kibana usages to the component

Update downstream snapshots

40c5f74

Remove all EuiText Sass

59d8695

Add changelog

e66e8c1

[PR feedback] EuiMarkdownFormat helper naming

d95cb50

cee-chen requested a review from thompsongl May 25, 2022 17:04

thompsongl reviewed May 25, 2022

View reviewed changes

src/components/markdown_editor/markdown_format.styles.ts Show resolved Hide resolved

cee-chen requested a review from cchaos May 25, 2022 19:28

thompsongl approved these changes May 25, 2022

View reviewed changes

Merge branch 'elastic:main' into emotion/eui-text

9653bb6

cchaos approved these changes May 26, 2022

View reviewed changes

upcoming_changelogs/5895.md Show resolved Hide resolved

[PR feedback] changelog

354566d

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

Merge branch 'main' into emotion/eui-text

30c1727

cee-chen mentioned this pull request May 26, 2022

Emotion/eui text 2 #5931

Closed

cee-chen enabled auto-merge (squash) May 26, 2022 19:51

cee-chen merged commit ca7d71f into elastic:main May 26, 2022

cee-chen deleted the emotion/eui-text branch May 26, 2022 20:18

elizabetdev mentioned this pull request Jun 2, 2022

[EuiText docs] Add callout with usage instructions #5946

Merged

2 tasks

thompsongl mentioned this pull request Jun 8, 2022

Upgrade EUI to v59.0.1 elastic/kibana#133927

Merged

cee-chen mentioned this pull request Jul 18, 2022

[EuiText] Fix multiple missing CSS logical properties #6059

Merged

3 tasks

thompsongl mentioned this pull request Aug 18, 2022

[EuiText] Updated euiTextContrainedMaxWidth Value #6146

Closed

4 tasks

nreese mentioned this pull request Aug 18, 2022

[Maps] fix scaling and term join in product help popover width elastic/kibana#139120

Merged

Conversation

cee-chen commented May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

QA

Uh oh!

Uh oh!

cee-chen May 11, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented May 11, 2022

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented May 17, 2022

Uh oh!

cchaos commented May 24, 2022

Uh oh!

kibanamachine commented May 24, 2022

Uh oh!

kibanamachine commented May 24, 2022

Uh oh!

cee-chen commented May 24, 2022

Uh oh!

cchaos commented May 25, 2022

Uh oh!

cee-chen commented May 25, 2022

Uh oh!

cee-chen commented May 25, 2022

Uh oh!

kibanamachine commented May 25, 2022

Uh oh!

thompsongl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented May 25, 2022

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cchaos commented May 26, 2022

Uh oh!

kibanamachine commented May 26, 2022

Uh oh!

kibanamachine commented May 26, 2022

Uh oh!

kibanamachine commented May 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cee-chen commented May 11, 2022 •

edited

Loading