-
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
New [EuiLoaderLogo] and other loader updates #4835
Conversation
if (icon) { | ||
iconNode = ( | ||
<> | ||
{icon} | ||
<EuiSpacer size="m" /> | ||
</> | ||
); | ||
} else if (iconType) { | ||
iconNode = ( |
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 is the only major change (adding the icon
prop) the rest just fixes the spacing.
Preview documentation changes for this PR: https://eui.elastic.co/pr_4835/ |
1 similar comment
Preview documentation changes for this PR: https://eui.elastic.co/pr_4835/ |
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! 🎉
Tested in Chrome, Safari, Edge, and Firefox.
/** | ||
* While this component should be restricted to using logo icons, it works with any IconType | ||
*/ | ||
logo?: IconType; |
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.
Should the logo prop be required rather than optional? This would make consumers always question the right logo to use rather than just using the default.
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.
Good question. Let me check what happens if its required but has a default. Because the Kibana one might be ok and would be redundant if they have to supply it.
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.
Just the one type change, otherwise LGTM.
FWIW, I'm ok with the logo
prop being optional and having logoKibana
as the default.
# Conflicts: # CHANGELOG.md
Whoops, sorry, forgot to push last week. Merging upstream now too. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4835/ |
Erg, a11y issues stemming from the new example using EuiPageTemplate. I'll get that fixed then merge. |
In EuiPageTemplate when `centeredContent` and no sidebar
Preview documentation changes for this PR: https://eui.elastic.co/pr_4835/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4835/ |
EuiLoadingLogo
This essentially replaces the old EuiLoadingKibana (now set for deprecation) component with a more generic one that accepts a
logo: IconType
.Pausing animations with
prefers-reduced-motion
is onThere may be more ways to enhance these loaders when the animations have been turned off, but for now it is important that we just pause them while users have this setting turned on.
You can test this yourself on Mac using the feature in the Accessibility settings:
Updated text content loader color to be lighter
Closes #3472
[EuiEmptyPrompt] Added
icon
prop passing a custom icon... And cleaned up spacing. I also updated the docs to present how this component can also be used for loading and error states, including how to use them with EuiPageTemplate.
Checklist
[ ] Checked for breaking changes and labeled appropriately[ ] Checked for accessibility including keyboard-only and screenreader modes([EuiLoading] Accessibility props #4814 has ideas to address a11y, should be a follow-up)