-
Notifications
You must be signed in to change notification settings - Fork 837
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] Convert the rest of the Loading components #5845
[Emotion] Convert the rest of the Loading components #5845
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_5845/ |
…ding_content # Conflicts: # src/components/loading/_index.scss
export const useLoadingAriaLabel = (): string => { | ||
return useEuiI18n('euiLoadingStrings.ariaLabel', 'Loading'); | ||
}; |
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.
Using one i18n string for all loading aria-labels
.
<span className="euiLoadingContent__singleLineBackground" /> | ||
</span> | ||
); | ||
lineElements.push(<span key={i} css={lineCssStyles} />); |
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 was able to reduce the DOM elements by using pseudo element ::after
instead
// Hide outline mainly for dark mode | ||
&:nth-of-type(1) { | ||
display: none; | ||
} |
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.
s: css` | ||
width: ${euiTheme.size[spinnerSizes.s]}; | ||
height: ${euiTheme.size[spinnerSizes.s]}; | ||
border-width: 1.5px; | ||
`, | ||
m: css` | ||
width: ${euiTheme.size[spinnerSizes.m]}; | ||
height: ${euiTheme.size[spinnerSizes.m]}; | ||
border-width: 1.5px; | ||
`, |
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.
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.
Much better! 😍
// TODO | ||
const sizeToClassNameMap = { | ||
m: 'euiLoadingLogo--medium', |
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 was going to do the same thing here as I did in EuiLoadingSpinner, but I need help with TS syntax.
* 2. Add a half the amount of animation distance padding to the top to give it more room | ||
*/ | ||
m: css` | ||
${euiCanAnimate} { | ||
/* 2 */ | ||
padding-block-start: calc( | ||
${euiTheme.size[loadingAnimationDistance.m]} / 2 | ||
); | ||
} |
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.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5845/ |
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.
It's looking good but I found one issue.
return { | ||
euiLoadingLogo__icon: css` | ||
${euiCanAnimate} { | ||
animation: 1s ${loadingBounce('m')} ${euiTheme.animation.resistance} |
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.
This animation is not working. The logos are not bouncing.
Screen.Recording.2022-04-27.at.01.37.54.PM.mp4
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.
Well this is fun... So on page load, the Emotion component styles will load within the global cache layer. But, the way that dev changes are injected, those styles are appended (comes after the CSS styles). @thompsongl , @chandlerprall I can see this happening a lot where we think something is working while we work on it, but on hard refresh it no longer is because the cascade order has changed.
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.
The animation should be fixed now :)
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.
where we think something is working while we work on it, but on hard refresh it no longer is because the cascade order has changed
Hmm I don't have any ideas for this at the moment. It's probably webpack, but I'd need to investigate further.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5845/ |
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.
Code LGTM! Just the one change.
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.
Thanks, @cchaos! Tested again in Chrome, Safari, Firefox, and Edge.
LGTM! 🎉
Preview documentation changes for this PR: https://eui.elastic.co/pr_5845/ |
Jenkins, test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5845/ |
Includes: EuiLoadingContent, EuiLoadingLogo, EuiLoadingElastic, and EuiLoadingSpinner
See further enhancements described in the comments below...
Checklist
[ ] Props have proper autodocs and playground toggles[ ] Added documentation[ ] Checked Code Sandbox works for any docs examples[ ] Checked for breaking changes and labeled appropriately[ ] Updated the Figma library counterpart