[Emotion] Convert `EuiIcon` by elizabetdev · Pull Request #5967 · elastic/eui

elizabetdev · 2022-06-14T15:44:36Z

Summary

This PR converts EuiIcon to Emotion.

I'm using withEuiTheme, WithEuiThemeProps so that I can pass the theme to emotion
Searched for euiIcon- to see if there was any usage and I could only find euiIcon-loaded being used in tests.
Because the file icon.tsx was too large due to the typeToPathMap object, I decided to move this object to its own file src/components/icon/icon_map.ts.
Some tests were updated (cached icons). Thanks, @constancecchen!
This PR doesn't convert the EuiToken to Emotion. I added a new entry in [Meta][CSS-in-JS] Component conversions #5685 to follow up on it.

Loading

The isLoading icon borders radius was updated to euiTheme.border.radius.small

Checklist

Checked in both light and dark modes
[ ] Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs and playground toggles
[ ] Added documentation
Checked Code Sandbox works for any docs examples
Added or updated jest and cypress tests
~~[ ] Checked for breaking changes and labeled appropriately~~
~~[ ] Checked for accessibility including keyboard-only and screenreader modes~~
~~[ ] Updated the Figma library counterpart~~
A changelog entry exists and is marked appropriately

Things to look out for when moving styles

~~[ ] Anything in the backlog that could be a quick fix for that component?~~
~~[ ] Convert global Sass vars to JS version like sizing~~
~~[ ] 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)
~~[ ] 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
Does the rendered class read as expected

elizabetdev · 2022-06-17T15:24:08Z

src/components/icon/icon.tsx

-        // The app icon only gets the .euiIcon--app class if no color is passed or if color="default" is passed
-        'euiIcon--app': isAppIcon && !appIconHasColor,
-        'euiIcon-isLoading': isLoading,
        'euiIcon-isLoaded': !isLoading && neededLoading,


In use in Kibana for tests.

Can we opt to replace this with a data-is-loaded attribute instead of a className (same for data-is-loading), and change the Kibana CSS overrides to target that data selector instead of the class?

kibanamachine · 2022-06-20T16:12:54Z

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

kibanamachine · 2022-06-20T16:32:15Z

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

- by checking for loading state rather than a basic component check

kibanamachine · 2022-06-20T18:16:38Z

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

kibanamachine · 2022-06-20T20:04:26Z

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

kibanamachine · 2022-06-21T13:07:32Z

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

elizabetdev · 2022-06-21T14:10:12Z

src/components/icon/icon.styles.ts

+    // TODO find out if this is still valid
+    // this focus was added 5 years ago
+    &:focus {
+      opacity: 1; // We often hide icons on hover. Make sure they appear on focus.
+      /* background: $euiFocusBackgroundColor; */
+    }


Does anyone know if this is still valid?

This how it would look like:

I think it's safe to remove the background-color.

is the opacity: 1 still valid as well? I can't personally think of a single scenario where we hide an icon on hover?

kibanamachine · 2022-06-21T14:45:33Z

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

src-docs/src/views/icon/icon_example.js

src-docs/src/views/icon/playground.js

kibanamachine · 2022-06-21T16:01:12Z

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

…pecific CSS

cee-chen

This is looking great so far! I pushed up some minor cleanup, but there's a few larger items (that may need more discussion or that you may have other feelings on) that I think still need to be addressed:

Re-adding custom colors as style and not passing it as an argument to the Emotion styles (#5967 (comment))
Deleting $euiIconLoadingOpacity and $euiIconColors (#5967 (comment))
Removing the euiIcon-isLoaded className in favor of a data attribute (#5967 (comment))
Removing &:focus { opacity: 1 }? (#5967 (comment))

kibanamachine · 2022-06-27T20:41:26Z

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

… variables file

kibanamachine · 2022-06-28T13:40:44Z

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

elizabetdev · 2022-06-28T14:22:40Z

Thanks, @constancecchen for reviewing this PR and for the cleanup. I went through your review and addressed the issues:

Re-added the custom colors as style 3b57233
Deleted $euiIconLoadingOpacity and $euiIconColors. And moved the icon sizes variables to the _form.scss file 509b890
Removed the euiIcon-isLoaded className in favor of a data attribute. I also added a data attribute for isLoading. I'm just not sure about the tests. I tried to update them. Let me know if it is the right way. ae58c9d
Removed &:focus { opacity: 1 } 193d9e3
Renamed the underscore functions and variables from icon.styles.ts 591acab

upcoming_changelogs/5967.md

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

src/components/icon/icon.tsx

cee-chen · 2022-06-28T15:55:31Z

Fantastic work with the changes!! :) Just 2 small suggestions/comments left and I think this is good to go!

kibanamachine · 2022-06-28T16:02:33Z

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

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

cee-chen

🎉 This looks really great! Thanks so much for taking my feedback!

kibanamachine · 2022-06-28T17:34:05Z

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

kibanamachine · 2022-06-28T17:43:33Z

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

Attempt to convert EuiIcon class comp to functional comp

6d02f3e

elizabetdev mentioned this pull request Jun 15, 2022

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

Closed

49 tasks

elizabetdev added 5 commits June 16, 2022 17:24

Merge remote-tracking branch 'upstream/main' into emotion-icon

f9337d2

Reverting to class comp. Better styles. Deleting sass icon files.

236007d

Merge remote-tracking branch 'upstream/main' into emotion-icon

f32b33b

Adding css class name in use

0801a19

Update test

0fa667b

elizabetdev commented Jun 17, 2022

View reviewed changes

elizabetdev added 2 commits June 20, 2022 16:46

Removing sass-lint important comments

09cbbbe

Adding shouldRenderCustomStyles

571039d

Fix/imporve appendIconComponentCache tests

d47ccba

- by checking for loading state rather than a basic component check

Showing props table

412aed9

Renaming icon map and better named_colors types

8b18907

Fix playground

d54c4fc

elizabetdev commented Jun 21, 2022

View reviewed changes

Removing console.log({ docgenInfo });

5482bb1

Passing custom color to Emotion

21fdcb1

elizabetdev commented Jun 21, 2022

View reviewed changes

src-docs/src/views/icon/icon_example.js Outdated Show resolved Hide resolved

elizabetdev commented Jun 21, 2022

View reviewed changes

src-docs/src/views/icon/playground.js Outdated Show resolved Hide resolved

Removing background color on focus

8ab9593

elizabetdev marked this pull request as ready for review June 21, 2022 15:43

elizabetdev requested review from cchaos and cee-chen June 24, 2022 12:57

cee-chen added 3 commits June 27, 2022 13:11

[PR feedback] Clean up ghost/text selectors as well as Elastic-logo s…

c398c4a

…pecific CSS

[PR feedback] comment cleanup

59ccf7a

[PR feedback] Export simplification

dd53f03

cee-chen suggested changes Jun 27, 2022

View reviewed changes

elizabetdev added 6 commits June 28, 2022 12:52

Merge remote-tracking branch 'upstream/main' into emotion-icon

c4861d0

Re-adding custom colors as style

3b57233

Renamed euiIconSizes to euiFormControlIconSizes and moved to form…

509b890

… variables file

Removing underscore from functions and variables

591acab

Removing unused .euiIcon:focus styles. Adding comments.

193d9e3

Adding data-is-loaded and data-is-loading attributes

ae58c9d

cee-chen reviewed Jun 28, 2022

View reviewed changes

upcoming_changelogs/5967.md Outdated Show resolved Hide resolved

Update upcoming_changelogs/5967.md

f07dd2c

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

cee-chen reviewed Jun 28, 2022

View reviewed changes

src/components/icon/icon.tsx Outdated Show resolved Hide resolved

elizabetdev and others added 2 commits June 28, 2022 17:47

Update src/components/icon/icon.tsx

8ed6ece

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

Adding customColor to show in className

2fcd282

cee-chen approved these changes Jun 28, 2022

View reviewed changes

Fixing path in creating-icons wiki

2466cba

elizabetdev enabled auto-merge (squash) June 28, 2022 17:19

elizabetdev merged commit b151438 into elastic:main Jun 28, 2022

1Copenut mentioned this pull request Jun 28, 2022

Bumping EUI to v60.1.2. elastic/kibana#135373

Merged

This was referenced Aug 9, 2022

[maps] draw icons not displayed in buttons elastic/kibana#138402

Closed

[Maps] fix draw icons not displayed in buttons elastic/kibana#138433

Merged

cee-chen mentioned this pull request Jan 27, 2023

[EuiIcon] Success color is a different hex value compared to other components #6556

Closed

Conversation

elizabetdev commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Loading

Checklist

Things to look out for when moving styles

QA

Uh oh!

elizabetdev Jun 17, 2022

Choose a reason for hiding this comment

Uh oh!

cee-chen Jun 27, 2022

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jun 20, 2022

Uh oh!

kibanamachine commented Jun 20, 2022

Uh oh!

kibanamachine commented Jun 20, 2022

Uh oh!

kibanamachine commented Jun 20, 2022

Uh oh!

kibanamachine commented Jun 21, 2022

Uh oh!

elizabetdev Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

cchaos Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

cee-chen Jun 27, 2022

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jun 21, 2022

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Jun 21, 2022

Uh oh!

cee-chen left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jun 27, 2022

Uh oh!

kibanamachine commented Jun 28, 2022

Uh oh!

elizabetdev commented Jun 28, 2022

Uh oh!

Uh oh!

Uh oh!

cee-chen commented Jun 28, 2022

Uh oh!

kibanamachine commented Jun 28, 2022

Uh oh!

cee-chen left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jun 28, 2022

Uh oh!

kibanamachine commented Jun 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

elizabetdev commented Jun 14, 2022 •

edited

Loading