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

add component property to spinner #1840

Closed
wants to merge 1 commit into from

Conversation

angorayc
Copy link

Summary

I would like to put a EuiLoadingSpinner in the title of EuiStat, but it complains about the p element from Stat shouldn't wrap div element from the spinner. Therefor I made a PR to make spinner accept component prop, so user can set it to div (default) or span. Please let me know if there's other solutions, any suggestions are welcome. Thanks a lot!

Screenshot 2019-04-16 at 22 11 49

Screenshot 2019-04-16 at 22 13 23

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
  • This required updates to Framer X components

@ryankeairns
Copy link
Contributor

ryankeairns commented Apr 16, 2019

Before looking into the code changes here, I was expecting to see the change on the EuiStat component as opposed to the EuiLoadingSpinner. Relatedly, a new EuiLoadingContent component was recently added and this has me thinking about a common pattern where we, say, pass some sort of isLoading prop on a component and the loading content is then displayed while true.

This pattern could then be followed on any components that need to show a loading state. Still thinking that through, but that is where my head is at. Is that making sense to others?

@angorayc
Copy link
Author

That's definitely a good idea to add isLoading prop to EuiStat, but in this case we probably should also think about which loading component we are going to match, not sure if spinner is suitable for other cases. It's more like a product's question to me, but I think it would be useful if the loading icon could be a span 😛

@snide
Copy link
Contributor

snide commented Apr 16, 2019

Agree with @ryankeairns that because this component should always load some sort of metric it might make sense to do this with an actual prop so that we have a consistent way to convey the action across usages (rather than each app using their own implementation). We can discuss in our design review today and figure something out. @MichaelMarcialis tends to show up to those as well and can make sure it works for SIEM.

@cchaos
Copy link
Contributor

cchaos commented Apr 16, 2019

I agree that I think both need to happen.

a. Add a loading state to EuiStat for consistency across all usages
b. Just change the EuiLoadingSpinner (and other loaders) to use spans with display: block (or inline-block with determined sizing so we don't keep running into this issue (I have also encountered it).

@angorayc
Copy link
Author

Thank you so much for these brilliant solutions, closing this PR as the changed has been covered by #1845.

@angorayc angorayc closed this Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants