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

Show all values in a series on mouse hover #2245

Merged
merged 4 commits into from
Sep 21, 2017

Conversation

jtarricone
Copy link
Contributor

Overview

This PR adds a new implementation of series "tooltips" that appear on all series items in a chart when any member of the series is hovered over. The actual elements added are not D3 tooltips, as it ultimately proved prohibitively difficult to implement alternative tooltip rendering. Rather, the elements are SVG objects drawn programmatically in such a way that they fulfill the designed role.

Connects #2158

Demo

recording 26

Notes

There's a bug/undesired rendering style on the charts showing the individual components of the combined hydrology. For bars that extend to the full height of the chart area, the tooltip element is cutoff, as shown here:

download 27

However, the Water Quality charts, which appear to be the same form of chart, do not have this issue:

download 28

After a few hours, I couldn't determine which D3 property or attribute was ultimately responsible for it. It looks like the runoff charts charting area is extended to the full possible area, whereas the water quality are not, and the clipping rules result in the former having clipped tooltip elements while the latter does not. I didn't have much luck beyond that.

Testing Instructions

  • rebundle, then open the app
  • set an AoI, do a model run, then add some scenarios and open the compare view
  • check that hovering over the charted items results in labels rendering for each item in that series
  • check that the hovered item is made more opaque than the others

@kellyi
Copy link
Contributor

kellyi commented Sep 12, 2017

Taking a look at this.

@kellyi
Copy link
Contributor

kellyi commented Sep 12, 2017

If one of the charts has a 0 value for the tooltip it looks like the tooltip skips rendering the text:

screen shot 2017-09-12 at 10 47 47 am

Wonder if we might be checking the value for falsiness such that 0 would fail? If so, may be worth checking something like if (_.isNumber(value)) instead of if (value)

parentG.append("text")
.each(function() {
d3.select(this).text(function() {
if (bar.y === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, here's where the zero values seem to get filtered out.

Copy link
Member

Choose a reason for hiding this comment

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

If these tooltips can't be hidden, I'd rather they say "0.00" than be empty

image

@kellyi
Copy link
Contributor

kellyi commented Sep 12, 2017

This seems to be working well so far! I tried it out in Chrome, FF, Safari, and IE11 and the tooltips generally rendered correctly aside from the runoff tooltips being cut off at the top.

Tagging @jfrankl here so he can take a look at this and also see if he has any idea why the water quality tooltips may render correctly at the top of the chart while the runoff tooltips don't. I wonder if it might be a z-index issue? I think we're generally using the same Marionette template & method, so most of the work of creating the chart would happen via d3/svg.

@jtarricone
Copy link
Contributor Author

jtarricone commented Sep 12, 2017

I wonder if it might be a z-index issue?

That's what I kept looking for, but I think D3 handles z-index in a pretty naive way by just drawing things in their procedural order. After staring at the charts and the associated markup I noticed that the water quality charts, as well as the combined hydrology chart, have a margin between the bars and the top of the rendered SVG, whereas the runoff charts don't appear to have one, so the tooltips at the top are clipped. I couldn't figure out why though, since the D3 instantiation appears to be the same.

@kellyi
Copy link
Contributor

kellyi commented Sep 12, 2017

I couldn't figure out why though, since the D3 instantiation appears to be the same.

Interesting -- not sure if this is still applicable but originally I was trying to make the chart y axis extend vertically further than the tops of the runoff bars by setting the tweaking the ydomain values to be like 120% of the max value from the data. I think we may have dropped that at some point, but maybe we brought something like it back in for the water quality charts?

Alternatively, I wonder if the wq labels might have some additional padding which encompasses the whole row?

@jtarricone
Copy link
Contributor Author

jtarricone commented Sep 12, 2017

Interesting -- not sure if this is still applicable but originally I was trying to make the chart y axis extend vertically further than the tops of the runoff bars by setting the tweaking the ydomain values to be like 120% of the max value from the data. I think we may have dropped that at some point, but maybe we brought something like it back in for the water quality charts?

That would probably result in this problem, but in that case would there be an obvious difference in the heights of the y-axis and of the tallest bar? That doesn't appear to be the case for the water quality charts. I can check the water quality chart instantiation to see if there's some padding added, which I don't recall specifically looking for.

@kellyi
Copy link
Contributor

kellyi commented Sep 15, 2017

Is this ready for another look?

@rajadain
Copy link
Member

Any movement on this?

@jtarricone
Copy link
Contributor Author

I was waiting on @jfrankl , but if design changes aren't required (yet) then I think this is ready to go. I didn't have any luck fixing the cropping issue, it's still a bit mysterious actually.

@rajadain
Copy link
Member

Taking a look at this.

"width": bgWidth + 2,
"height": bgHeight + 2,
"stroke": 1,
"class": ("bar-value-text-bg-shadow_" + el)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of appending with an underscore, prefer appending with a space so these elements can be targeted via CSS like .bar-value-test-bg-shadow

Copy link
Member

Choose a reason for hiding this comment

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

diff --git a/src/mmw/js/src/core/chart.js b/src/mmw/js/src/core/chart.js
index a462dbe..508e071 100644
--- a/src/mmw/js/src/core/chart.js
+++ b/src/mmw/js/src/core/chart.js
@@ -437,7 +437,7 @@ function renderCompareMultibarChart(chartEl, name, label, colors, stacked, yMax,
                                     "width": bgWidth + 2,
                                     "height": bgHeight + 2,
                                     "stroke": 1,
-                                    "class": ("bar-value-text-bg-shadow_" + el)
+                                    "class": ("bar-value-text-bg-shadow " + el)
                                 });
 
                                 d3.select(this).style({
@@ -459,7 +459,7 @@ function renderCompareMultibarChart(chartEl, name, label, colors, stacked, yMax,
                                     "height": bgHeight,
                                     "stroke": "#aaaaaa",
                                     "stroke-width": 0.5,
-                                    "class": ("bar-value-text-bg_" + el)
+                                    "class": ("bar-value-text-bg " + el)
                                 });
 
                                 d3.select(this).style({
@@ -481,7 +481,7 @@ function renderCompareMultibarChart(chartEl, name, label, colors, stacked, yMax,
                                     "x": (parseFloat(b.attr("x")) + (parseFloat(barWidth) / 2)),
                                     "width": dialogTickSize,
                                     "height": dialogTickSize,
-                                    "class": ("bar-value-text-bg-tick_" + el)
+                                    "class": ("bar-value-text-bg-tick " + el)
                                 });
 
                                 d3.select(this).style({
@@ -510,7 +510,7 @@ function renderCompareMultibarChart(chartEl, name, label, colors, stacked, yMax,
                                     "font-size": "0.8rem",
                                     "font-weight": "normal",
                                     "font-family": "helvetica",
-                                    "class": ("bar-value-text_" + el)
+                                    "class": ("bar-value-text " + el)
                                 });
 
                                 d3.select(this).style({
@@ -531,21 +531,21 @@ function renderCompareMultibarChart(chartEl, name, label, colors, stacked, yMax,
                     .selectAll("rect.nv-bar")
                     .on("mouseover", function () {
                         d3.select("#" + chartContainerId)
-                            .selectAll(".bar-value-text_" + el)
+                            .selectAll(".bar-value-text." + el)
                             .style("opacity", "1");
 
                         d3.select("#" + chartContainerId)
-                            .selectAll(".bar-value-text-bg-shadow_" + el)
+                            .selectAll(".bar-value-text-bg-shadow." + el)
                             .style("opacity", "0.5")
                             .style("fill-opacity", "0.5");
 
                         d3.select("#" + chartContainerId)
-                            .selectAll(".bar-value-text-bg_" + el)
+                            .selectAll(".bar-value-text-bg." + el)
                             .style("opacity", "1")
                             .style("fill-opacity", "1");
 
                         d3.select("#" + chartContainerId)
-                            .selectAll(".bar-value-text-bg-tick_" + el)
+                            .selectAll(".bar-value-text-bg-tick." + el)
                             .style("opacity", "1")
                             .style("fill-opacity", "1");
                     });
@@ -556,21 +556,21 @@ function renderCompareMultibarChart(chartEl, name, label, colors, stacked, yMax,
                     .selectAll("rect.nv-bar")
                     .on("mouseout", function () {
                         d3.select("#" + chartContainerId)
-                            .selectAll(".bar-value-text_" + el)
+                            .selectAll(".bar-value-text." + el)
                             .style("opacity", "0");
 
                         d3.select("#" + chartContainerId)
-                            .selectAll(".bar-value-text-bg-shadow_" + el)
+                            .selectAll(".bar-value-text-bg-shadow." + el)
                             .style("opacity", "0")
                             .style("fill-opacity", "0");
 
                         d3.select("#" + chartContainerId)
-                            .selectAll(".bar-value-text-bg_" + el)
+                            .selectAll(".bar-value-text-bg." + el)
                             .style("opacity", "0")
                             .style("fill-opacity", "0");
 
                         d3.select("#" + chartContainerId)
-                            .selectAll(".bar-value-text-bg-tick_" + el)
+                            .selectAll(".bar-value-text-bg-tick." + el)
                             .style("opacity", "0")
                             .style("fill-opacity", "0");
                     });

Copy link
Member

Choose a reason for hiding this comment

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

We can then do something like this:

diff --git a/src/mmw/sass/pages/_compare.scss b/src/mmw/sass/pages/_compare.scss
index 3ebf563..d73574b 100644
--- a/src/mmw/sass/pages/_compare.scss
+++ b/src/mmw/sass/pages/_compare.scss
@@ -463,6 +463,13 @@ $compare-chart-table-height: calc(100vh - #{$height-lg} - #{$compare-footer-heig
     rect {
       transition: opacity 0.3s ease-in-out;
     }
+
+    .bar-value-text-bg-shadow,
+    .bar-value-text-bg,
+    .bar-value-text-bg-tick,
+    .bar-value-text {
+      pointer-events: none;
+    }
   }
 
   .compare-scenario-gradient {

Which allows hovering on the stacked chart in areas obscured by hidden tooltips to function correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason it's so specific is that those elements have to be manipulated via D3 later on. Could D3 selection be performed (easily) with a lower degree of specificity in the elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did that CSS get tested on the D3 charts? I observed that CSS and D3/SVG elements didn't seem to play very nice together.

Copy link
Member

Choose a reason for hiding this comment

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

As shown in the diff, a CSS class of abcd_wxyz can be accessed only via .abcd_wxyz, but one of abcd wxyz can be accessed specifically with .abcd.wxyz, but also in other groupings with .abcd and .wxyz. So the existing selection behavior is preserved by switching to spaces and dots instead of the underscore, and enables this CSS targeting after the fact.

@rajadain
Copy link
Member

With these suggested fixes, I think this'll be good to go. We can make a follow up card that looks into the tooltips being cutoff for high values, and for removing them for 0 values.

@jtarricone
Copy link
Contributor Author

I pushed fixes but I'm having trouble getting my environment spun up, so I haven't tested yet. I'll loop back once I test it, unless you get a chance before I do.

Copy link
Member

@rajadain rajadain left a comment

Choose a reason for hiding this comment

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

This is working pretty well for me:

2017-09-21 11 56 07

@rajadain
Copy link
Member

Made #2278 for follow up improvements.

@jtarricone
Copy link
Contributor Author

Unless @kellyi has any objection I'll merge once the build (successfully) completes.

@jtarricone jtarricone assigned jtarricone and unassigned kellyi Sep 21, 2017
@jtarricone jtarricone merged commit 3b8a4de into develop Sep 21, 2017
@rajadain rajadain deleted the feature/jmt/compare-chart-tooltips branch October 6, 2017 21:12
@rajadain rajadain mentioned this pull request Oct 16, 2017
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.

None yet

3 participants