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

Stats: Create Placeholder Component #2491

Merged
merged 3 commits into from
Jan 20, 2016
Merged

Stats: Create Placeholder Component #2491

merged 3 commits into from
Jan 20, 2016

Conversation

timmyc
Copy link
Contributor

@timmyc timmyc commented Jan 15, 2016

Currently the markup for rendering placeholder/skeleton loading indicators is scattered throughout all stats components. This branch aims to centralize all of that markup/scss into one central location, and to fix some behaviors around it.

To Test
The best way to test the placeholders is to empty your localStorage on the Insights page, and a year view of a site stats page. Refresh the pages on empty local storage and ensure everything still has pulsing loading indicators, and after 2s the spinners should appear.

To Do

/cc @alternatekev and @jancavan for thoughts

@timmyc timmyc added [Feature] Stats Everything related to our analytics product at /stats/ [Status] In Progress labels Jan 15, 2016
@timmyc timmyc self-assigned this Jan 15, 2016
@timmyc timmyc added this to the Stats: Maintenance milestone Jan 15, 2016
@timmyc timmyc added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jan 19, 2016
@jblz
Copy link
Member

jblz commented Jan 19, 2016

Code refactor all looks good to me.

When I test using the instructions, though, I only see the placeholders flash on for a split-second & no spinners at all.

Here's a video:
https://cloudup.com/c2umZtLAZtU

That TypeError is an existing issue here that I'm working on in #2459

@jancavan
Copy link
Contributor

In all stats modules, the placeholder pulsing skeletons are replaced after six seconds by a spinner. Lets reduce the spinner display time to 3 seconds instead.

I tested this branch and the spinner and skeletons both show up at the same time?

screenshot-2016-01-19-15 21 59

Is there a reason why we show 2 different preloaders consecutively? I think we should just show one or the other - at least for all the modules, except for the bar chart which can retain the spinner.

@timmyc
Copy link
Contributor Author

timmyc commented Jan 20, 2016

I believe the history behind the spinner was some stats modules were so slow to load ( like the chart on long periods ) that the spinner would be shown after the 6 seconds. The skeleton and spinner are shown at the same time on production right now.

The only ( planned ) behavior change in this branch is to show the spinner sooner that it currently happens in production.

From my testing, the branch behaves like production does, but I could have just been staring at it too long today.

@jancavan
Copy link
Contributor

Yeah, probably out of scope for this cleanup. But I can confirm that the spinner shows up ~2 sec after the skeleton 👍

@timmyc
Copy link
Contributor Author

timmyc commented Jan 20, 2016

@jancavan so good to 🚢 this? @jblz any other feedback?

@jancavan
Copy link
Contributor

@timmyc 👍 I'll just create a different issue for the double spinners preloaders.

@jblz jblz added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 20, 2016
@jblz
Copy link
Member

jblz commented Jan 20, 2016

Marking this ready to go as I'm seeing a similar effect w/ empty localStorage on other branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Stats Everything related to our analytics product at /stats/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stats: Reduce Spinner Display Time
4 participants