-
Notifications
You must be signed in to change notification settings - Fork 841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Emotion] Converted EuiBeacon #5814
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_5814/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5814/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5814/ |
@@ -3,7 +3,7 @@ | |||
exports[`EuiBeacon accepts size 1`] = ` | |||
<div | |||
aria-label="aria-label" | |||
class="euiBeacon testClass1 testClass2" | |||
class="euiBeacon testClass1 testClass2 css-haebed-euiBeacon-euiCanAnimate-euiBeacon-euiCanAnimate-euiBeacon" |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5814/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5814/ |
miukimiu#29 is one idea for fixing |
Change `euiCanAnimate` to a constant
Preview documentation changes for this PR: https://eui.elastic.co/pr_5814/ |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
Preview documentation changes for this PR: https://eui.elastic.co/pr_5814/ |
Summary
This PR converts the EuiBeacon to Emotion.
prefers-reduced-motion: no-preference
.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-inline
and-block
(Logical properties)[ ] Reduce specificity where possible (usually by reducing class name chaining)[ ] Usegap
property to add margin between items if using flex[ ] Can any still existing.js
files be converted to TS?QA
className
,css
, andstyle
props work properly when passed by the consumerChecklist
[ ] Checked in mobile[ ] Added documentation[ ] Checked Code Sandbox works for any docs examples[ ] Checked for breaking changes and labeled appropriately[ ] Checked for accessibility including keyboard-only and screenreader modes[ ] Updated the Figma library counterpart