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

Issue 857 - Chart loading skeleton #858

Merged
merged 16 commits into from
Sep 7, 2019
Merged

Issue 857 - Chart loading skeleton #858

merged 16 commits into from
Sep 7, 2019

Conversation

jaronheard
Copy link
Contributor

Resolves #857.

This adds a loading skeleton to charts.

Prior to this PR, charts did not have a height before they were loaded, so it caused a page reflow.

Before After
Screen Recording 2019-08-28 at 2 25 55 AM Screen Recording 2019-08-28 at 2 22 22 AM

That said, there's a number of things on this PR that I'm not especially confident in.

  • This adds a dependency on @material-ui/labs, as the Skeleton component is not yet in the @material-ui/core. Rather than add a dependency, should we just implement this as an individual component?
  • Should the Skeleton be its own component? Or is it ok to just have it be implemented in ChartWrapper?
  • I added a check for safeData to avoid having the charts throw errors if data is null and isLoading is true, but it smells fishy to me.


if (loading) {
content = <div css={chartLoading}>Loading...</div>;
content = <Skeleton css={[wrapperStyle, fullHeight]} />;
Copy link

Choose a reason for hiding this comment

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

This should be default! Then it fixes all the problems, maybe... no weird safe data checks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this but unfortunately some kind of weird safeData checks are still required because of the way the components are structured. There's probably another cleaner way to fix this -- after demo day

@ghost
Copy link

ghost commented Aug 29, 2019

We should have the isLoading prop in all visualizations so we don't have to do the conditional chart mount which causes a layout jump. If that's done here, I'll update the cards in #873 to use it, otherwise if mine is merged first we'll need to do a sweep later and update them.

@jaronheard jaronheard requested a review from a user August 29, 2019 09:00
@jaronheard jaronheard assigned ghost Aug 30, 2019
@ghost
Copy link

ghost commented Aug 31, 2019

Height ratio is off. Also, to keep this user friendly we should show the user something besides just a grayish rectangle. Could be as simple as some words or a loading spinner...

@jaronheard
Copy link
Contributor Author

Current status for this PR:

It adds a note about a newly discovered issue with LineChart, which will be covered in another PR:
If there are lines that have different numbers of data points, then they will be colored incorrectly.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Loading "skeleton" ratio now works and has a spinner to indicate somethings happening. Tests and documentation look good for protectData

@jaronheard jaronheard merged commit 643a4e0 into master Sep 7, 2019
@jaronheard jaronheard deleted the issue-857 branch September 7, 2019 04:55
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.

Implement MaterialUI skeleton for chart loading
1 participant