Skip to content
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

Merged
merged 12 commits into from
Jun 1, 2021

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented May 27, 2021

EuiLoadingLogo

This essentially replaces the old EuiLoadingKibana (now set for deprecation) component with a more generic one that accepts a logo: IconType.

Screen Shot 2021-05-27 at 17 19 38 PM

Pausing animations with prefers-reduced-motion is on

There 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:
Screen Shot 2021-05-27 at 11 35 57 AM

Updated text content loader color to be lighter

Closes #3472

image

[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.

Screen Shot 2021-05-27 at 17 24 23 PM

Checklist

  • Check against all themes for compatibility 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 the any docs examples
  • Added or updated jest tests
  • [ ] 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)
  • A changelog entry exists and is marked appropriately

Comment on lines +80 to +88
if (icon) {
iconNode = (
<>
{icon}
<EuiSpacer size="m" />
</>
);
} else if (iconType) {
iconNode = (
Copy link
Contributor Author

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.

@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

@cchaos cchaos mentioned this pull request May 27, 2021
33 tasks
@cchaos cchaos added the deprecations Contains deprecations. Add them to the deprecations meta ticket after merge. label May 27, 2021
Copy link
Contributor

@miukimiu miukimiu left a 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;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, unfortunately, even with a default value TS complains.

Screen Shot 2021-05-28 at 14 52 48 PM

src/components/empty_prompt/empty_prompt.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@thompsongl thompsongl left a 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.

src/components/loading/loading_logo.tsx Outdated Show resolved Hide resolved
@cchaos
Copy link
Contributor Author

cchaos commented Jun 1, 2021

Whoops, sorry, forgot to push last week. Merging upstream now too.

@cchaos cchaos requested a review from thompsongl June 1, 2021 16:28
@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor Author

cchaos commented Jun 1, 2021

Erg, a11y issues stemming from the new example using EuiPageTemplate. I'll get that fixed then merge.

In EuiPageTemplate when `centeredContent` and no sidebar
@kibanamachine
Copy link

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

@cchaos cchaos merged commit aff9828 into elastic:master Jun 1, 2021
@kibanamachine
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Contains deprecations. Add them to the deprecations meta ticket after merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiLoadingContent] Make indicators lighter?
4 participants