Skip to content

[Emotion] Converted EuiBeacon#5814

Merged
elizabetdev merged 18 commits intoelastic:mainfrom
elizabetdev:beacon-emotion
Apr 22, 2022
Merged

[Emotion] Converted EuiBeacon#5814
elizabetdev merged 18 commits intoelastic:mainfrom
elizabetdev:beacon-emotion

Conversation

@elizabetdev
Copy link
Copy Markdown
Contributor

@elizabetdev elizabetdev commented Apr 19, 2022 •
edited
Loading

Summary

This PR converts the EuiBeacon to Emotion.

  • The animation now only runs if prefers-reduced-motion: no-preference.
  • Added a test shouldRenderCustomStyles()

How to test

You can test by comparing the new implementation with the old one.

Beacon:

Beacon in tour component:

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?

QA

  • Do className, css, and style props work properly when passed by the consumer
  • Does the rendered class read as expected

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

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 19, 2022

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

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 19, 2022

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

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 20, 2022

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

@elizabetdev elizabetdev changed the title [Draft] Convert EuiBeacon to Emotion [Emotion] Converted EuiBeacon Apr 20, 2022
@elizabetdev elizabetdev marked this pull request as ready for review April 20, 2022 13:25
cchaos
cchaos reviewed Apr 20, 2022
View reviewed changes
src/components/beacon/__snapshots__/beacon.test.tsx.snap Outdated
<div
aria-label="aria-label"
class="euiBeacon testClass1 testClass2"
class="euiBeacon testClass1 testClass2 css-haebed-euiBeacon-euiCanAnimate-euiBeacon-euiCanAnimate-euiBeacon"
Copy link
Copy Markdown
Contributor

@cchaos cchaos Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why this one is outputting -euiBeacon so many times...

Copy link
Copy Markdown
Contributor Author

@elizabetdev elizabetdev Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested and it is because of the :before and :after. I also showed it to @chandlerprall and he will take a look while trying to improve the euCanAnimate mixin.

elizabetdev
elizabetdev commented Apr 20, 2022
View reviewed changes
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 20, 2022

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

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 20, 2022

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

@thompsongl thompsongl mentioned this pull request Apr 20, 2022
Closed
49 tasks
@thompsongl
Copy link
Copy Markdown
Contributor

thompsongl commented Apr 20, 2022

elizabetdev#29 is one idea for fixing euiCanAnimate

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 21, 2022

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

@elizabetdev elizabetdev requested a review from cchaos April 21, 2022 12:59
cchaos
cchaos approved these changes Apr 21, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Love the stopped-motion version. 😍

upcoming_changelogs/5814.md Outdated
@@ -0,0 +1,3 @@
**CSS-in-JS conversions**

- Converted `EuiBeacon` to Emotion No newline at end of file
Copy link
Copy Markdown
Contributor

@cchaos cchaos Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a CL item that says the canAnimate hooks have been changed to a constant?

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 21, 2022

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

@elizabetdev elizabetdev merged commit 3c427b1 into elastic:main Apr 22, 2022
@1Copenut 1Copenut mentioned this pull request Apr 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@cee-chen cee-chen cee-chen left review comments

@cchaos cchaos cchaos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@elizabetdev @kibanamachine @thompsongl @cee-chen @cchaos