Compare View: TR-55 Runoff Charts #2123
Conversation
kellyi
force-pushed
the
ki/compare-view-tr55-runoff
branch
12 times, most recently
from
90000e9
to
fd3bb5e
Compare
kellyi
force-pushed
the
ki/compare-view-tr55-runoff
branch
from
fd3bb5e
to
6ce8300
Compare
|
Realized I left off a change which forces the y axis for the non-Combined-Hydrology charts to go slightly beyond the max value of the bars:
Amended the commit & pushed it anew. |
kellyi
changed the title
This is one of our few projects to which this doesn't apply. |
Whoops! |
|
The tick labels on the Combined Hydrology chart are incorrect:
This should be 10 instead of 1. The labels on the other charts are correct. |
src/mmw/js/src/compare/views.js
Outdated
| var self = this; | ||
| this.model.get('scenarios').forEach(function(scenario) { | ||
| scenario.get('results').on('change', function(s) { | ||
| if (s.attributes.name === 'runoff' && !s.attributes.polling) { |
There was a problem hiding this comment.
We should still use the backbone syntax: s.get('name') and s.get('polling'). We also may want to refer to it as r, for "result"
| @@ -472,8 +534,74 @@ function getTr55Tabs(scenarios) { | |||
| }) | |||
| }, | |||
| ], | |||
| // TODO Make Runoff charts | |||
| runoffCharts = [], | |||
| runoffCharts = [ | |||
There was a problem hiding this comment.
@rajadain Was there a reason for all the arrays/objects in this function to not be actual models?
Why doesn't this return aTabsCollection?
Good catch. The combined hydrology chart needs to get the precipitation slider value as an input so it can use it to set its |
|
This is interesting: when the precipitation value is changed, we run a TR-55 job for all four scenarios (in this example), and update the chart as the values return. As the initial values return, there is great variation in the chart, as we have some new low values coexisting with high old values. Once all the new ones are gathered, the chart rescales itself to the new standard. This is not very obvious to the user (especially because there are no spinners or loading symbols, more on that later), but the effect can be jarring:
Can we make it so that the chart updates when every scenario has finished running instead of individual ones? |
|
In the wireframes: https://marvelapp.com/8hi9jg9/screen/29125398, the background color of the columns alternates. Was any thought given to this here? |
|
It looks like all the edge cases I found have been noted above in the description. Otherwise this works well, and moves us closer. Going to take a look at the code now. |
Whoops, missed that one. Should be simple enough to do with index mod 2 though, so I can add a commit to fix it. Working on the combined hydrology ydomain value, too. |
src/mmw/js/src/core/chart.js
Outdated
| @@ -346,6 +346,8 @@ function renderCompareMultibarChart(chartEl, name, label, colors, stacked, data) | |||
| if (maxValue) { | |||
| yDomainMax = maxValue * 1.2; | |||
| } | |||
| } else { | |||
| yDomainMax = precipitation || 1; | |||
There was a problem hiding this comment.
The fallback's to ensure that if precipitation's set to 0, the graph will still range from 0 to 1.
|
8a046c3 departs from the wireframes to show the max value on the y-axis:
If we decide we don't want to show them, I can either drop the commit or we can set |
|
0.1 is shown twice in different locations in the Evapotranspiration chart in the screenshot. That can't be right. |
Ah, looks like we have to set the tick count differently if there are only two values. |
- add chart row views & collection - add compareChartRow template - structure charts data in `getTR55Tabs` function - create renderCompareMultibarChart function - format data & other arguments properly in addChart fn - style compare charts to match scenario cards in title
kellyi
force-pushed
the
ki/compare-view-tr55-runoff
branch
from
2ffae03
to
dc0d178
Compare
|
I squashed this down to one commit & rebased it on |
|
See #2134 (comment) |
|
Closing in favor of #2148. |
Overview
This PR adds the TR-55 Compare View charts and sets up the table & chart values to adjust accordingly. gif demo's below.
Connects #2072
Demo
Notes
Currently there are a few quirks & limitations with this implementation, which we can either handle here or spin off elsewhere.
I did not implement the popovers, which seem to be complicated enough to handle in another card. NVD3 has tooltips by default, but since the desired behavior's that the hovering over one value on the bar chart opens corresponding popovers for all three, which isn't a built-in NVD3 feature. I think we may end up wanting to use Bootstrap popovers for this.
The bars themselves match the width of the scenario title placeholders when there are few enough of them to display in the scenario header without overflow. However, when there are more, the chart div seems not to expand in size beyond what's visible so the bars get squished.
There's currently some flicker as new charts are shown when retrieving any of the results, which means it renders once per scenario if the precipitation slider value changes. Not sure what is the best way to handle this -- I think we may want to set some value to check whether any of the scenarios is currently polling and show a loading state if not?
I think the maximum y axis value for "Combined Hydrology" should be the value the precipitation slider has been set to, but it seemed like the value these scenarios had was slightly off the number displayed for the slider. Not sure what is the best way to handle this, but we'll need to pass it in to the chart to set the
yDomain.Happy to try to address these here or to spin them off elsewhere; figured I'd put up the PR for review rather than spinning wheels on it.
Testing
updateandserverbundle.sh& hard refresh in the browser