Compare View Revamp: Loading States #2226
Conversation
rajadain
force-pushed
the
tt/compare-view-revamp-loading-states
branch
from
9b376e9
to
33dc0cf
Compare
|
Taking a look at this. |
|
Working well in Chrome, FF, and Safari! In IE 11 I don't see the loading spinner -- and the stacked bar chart's not showing up. I think the latter's a known issue, so depending on whether we want to address the loading spinner icon here or not, this looks good to me. |
|
I did notice once quirk with the compare view slider which probably merits another issue if it's not the intended behavior: viz, it looks like the slider button allows you to move the scenario columns once even if they all fit into the viewport:
->
If this isn't intentional, we should make another issue to fix it. |
src/mmw/js/src/compare/models.js
Outdated
| return scenario.get('results').pluck('polling'); | ||
| }, | ||
| scenarios = this.get('scenarios'), | ||
| polling = _.flatten(scenarios.map(getPolling)).some(_.identity); |
There was a problem hiding this comment.
Probably fine to leave but this could also use the lodash/underscore version of some?
|
@jfrankl I'm concerned that the loading spinner itself is too subtle - I didn't even notice it for a minute in the GIF on this PR. I think the lightening of the rest of the panel is helpful, but the spinner is very small and subtle. Perhaps we could consider doing something larger, potentially positioned over the greyed-out area? |
In #2195 I added a |
|
Going to pull the new commit down and run this again before approving. |
There was a problem hiding this comment.
+1 from me. I tested this out again and it still works well after adding lodash some.
Probably worth waiting on @ajrobbins and @jfrankl to +1 the loading indicator appearance, too, but functionally this is working well.
|
Created #2236 for the sliding issue described above. |
|
Two suggestions that I think will improve the visibility of the spinner.
Also, please make the following changes so the application matches the specifications in #2134 (comment):
|
|
I think we should use the new spinner, and later add it to the modeling sidebar. Since this is a new component, I'm more confident about including a new visual element here. With the new wave spinner, should it also be centered? I'll await the wave spinner CSS on Slack, will implement the rest now. |
|
Yes, I think it should also be centered. |
|
Pushed up some fixups. This is what it looks like now:
I was unable to remove the opacity transition from the bar charts, because we also use it for hiding them as they slide. But they look pretty snappy to me ... |
|
Looking good to me. |
|
Great. I'll squash and merge once @jfrankl has had a chance to take a look. |
|
Squashing and merging now. |
We introduce a new `polling` boolean attribute on the Compare Window model, false by default, which reflects whether the window is currently in a state of polling or not. This is contingent on any of the scenarios having any results that are polling. To do this, we first collect all the `polling` attributes in each of the result in each scenario, and then flatten it to a single array. Then we use `Array.some` which takes a function and returns true if it evaluates to a truthy value for any of the values. Since the values are already boolean, we simply use `_.identity` as the evaluating function.
When the Window Model has `polling` set to true, the sections region containing charts or tables will appear faded. It will come back into saturation once the polling stops. This change is animated in the same manner as all other animations in the Compare Window. This does not impede user interactivity. They can still hover over the charts and select text in tables.
Add a spinner on the top middle, left of the precipitation slider, to be shown when the scenarios are polling. This uses a new spinner which is sleeker and faster than the usual fa-circle-o-notch used elsewhere in the app. Eventually, all of those should be replaced with this. squash! Add spinner on poll The spinner is now in the middle of the screen, rather than the top-right.
rajadain
force-pushed
the
tt/compare-view-revamp-loading-states
branch
from
b812122
to
d1fc31f
Compare
|
Thanks for all the feedback and support! |
Overview
Adds loading states to indicate that the models are being run. All results are faded, and a spinner is added at the top-right corner. Interactivity is not impacted at all. See commit messages for details.
Connects #2134
Demo
Testing Instructions
bundle