Set max width for bar chart labels #141 by johnmhoran · Pull Request #149 · aboutcode-org/scancode-workbench

johnmhoran · 2017-08-07T17:43:17Z

Changes:
- Bar chart label text is limited to 50 characters (plus “. . .”
  for those strings that exceed 50 and are therefore abbreviated).
  Particularly noticeable (at least on “zlib-1.2.11” test scan)
  for the “Copyright Statements” selection.
- Tooltip is displayed on mousing over the bar for all bar chart
  selections.
- Tooltip displays the label text plus the count in parentheses.
- Replace '<' with '<' before calling tooltip -- for some reason
  the open angle-bracket interrupts the tooltip display (the close
  angle-bracket seems to display OK as is).
- Chart dropdown no longer has a visible focus outline/border after
  a selection is made.
Open issues:
- Adding a tooltip for the bar chart labels would be helpful since
  on some display scales, the bar chart bar for some items is small
  and triggering the tooltip requires precise placement. (I
  researched and experimented extensively but could not figure out
  how to accomplish this.)
- The “Copyright Authors” selection is displayed on launch but not
  shown as selected in the dropdown’s default display. (This
  behavior existed before adding the tooltip.)
- The margin/whitespace between the top of the vertical axis and
  the top-most item increases as the number of displayed items
  increases. Similar behavior for the bottom of the axis. This is
  most pronounced (on the “zlib-1.2.11” test scan) for the
  “Copyright Statements” selection (73 items displayed). (This
  behavior existed before adding the tooltip.)

Signed-off-by: John M. Horan johnmhoran@gmail.com

* Changes: * Bar chart label text is limited to 50 characters (plus “. . .” for those strings that exceed 50 and are therefore abbreviated). Particularly noticeable (at least on “zlib-1.2.11” test scan) for the “Copyright Statements” selection. * Tooltip is displayed on mousing over the bar for all bar chart selections. * Tooltip displays the label text plus the count in parentheses. * Replace '<' with '<' before calling tooltip -- for some reason the open angle-bracket interrupts the tooltip display (the close angle-bracket seems to display OK as is). * Chart dropdown no longer has a visible focus outline/border after a selection is made. * Open issues: * Adding a tooltip for the bar chart labels would be helpful since on some display scales, the bar chart bar for some items is small and triggering the tooltip requires precise placement. (I researched and experimented extensively but could not figure out how to accomplish this.) * The “Copyright Authors” selection is displayed on launch but not shown as selected in the dropdown’s default display. (This behavior existed before adding the tooltip.) * The margin/whitespace between the top of the vertical axis and the top-most item increases as the number of displayed items increases. Similar behavior for the bottom of the axis. This is most pronounced (on the “zlib-1.2.11” test scan) for the “Copyright Statements” selection (73 items displayed). (This behavior existed before adding the tooltip.) Signed-off-by: John M. Horan <johnmhoran@gmail.com>

Signed-off-by: John M. Horan <johnmhoran@gmail.com>

jdaguil · 2017-08-08T05:50:05Z

        // Create scaling for y that converts formattedData names to pixels
        let yScale = d3.scale.ordinal()
-            .domain(formattedData.map(function(d) {return d.name; }))
+            .domain(formattedData.map(function(d) {return d.trimName; }))


I think we want to use the full name here. Consider the case where two names only differ on character 51 and we truncate on character 50.

[ "some...long...name1" "some...long...name2" ]

After truncation we would get:

[ "some...long...name" "some...long...name" ]

Each bar in the chart needs a distinct name in the y-axis "domain" in order to be drawn at a distinct height along the y-axis. In the case above, there would be no way for the y axis to distinguish the names, and you would probably get overlapping bars at that same position instead of two different bars at different heights.

It looks like one possible solution might use be to use the d3 axis's .tickFormat() to set the names of the labels different from the domain of the y-axis.

jdaguil · 2017-08-08T05:50:25Z

+}
+
+/* Tooltip for Bar Chart */
+ .toolTip {


nit: remove extra space.

jdaguil · 2017-08-08T06:00:49Z

        // Transform license count into array of objects with license name & count
        let chartData = $.map(count, function(val, key) {
+            let trimName = "";
+            if (key.length > 50) {


style nit: you could use a ternary operator here:

trimName = key.substring(0, 50) + (key.length > 50 ? " ..." : "");

jdaguil · 2017-08-08T06:07:16Z

-            .attr('height', yScale.rangeBand());
+            .attr('y', function(d) { return yScale(d.trimName); })
+            .attr('height', yScale.rangeBand())
+            .on("mouseover", function (d) { tooltip.style("display", "inline-block"); })


We should also have a tooltip on the y-axis label. I've noticed some cases where the bar is so small that there's actually no way to trigger the tooltip.

jdaguil

Nice feature!

* Update local branch with changes to 'develop'. Signed-off-by: John M. Horan <johnmhoran@gmail.com>

* Bar chart labels now have tooltips with same content as corresponding bars. * Changed from truncated name to full name as appropriate. Signed-off-by: John M. Horan <johnmhoran@gmail.com>

jdaguil

Added a few comments. :) Also, can you merge develop into your branch?

jdaguil · 2017-08-15T02:28:40Z


        // Transform license count into array of objects with license name & count
        let chartData = $.map(count, function(val, key) {
+            let trimName = "";


Assigning to an empty string is unneeded. Instead do this:

let trimName = key.substring(0, 50) + (key.length > 50 ? " ..." : "");

Done -- using the modified variable name and the new static method:

let trimmedName = BarChart.trimName(key);

jdaguil · 2017-08-15T02:33:47Z

            .scale(yScale)
+            // Limit label length to 50 characters plus ellipses.
+            .tickFormat(function(d) {
+                return d.substring(0, 50) + (d.length > 50 ? " ..." : "");


This same logic is defined below. Can you refactor this out into a function?

Good point as always @jdaguil . I've created a static method: static trimName(name) {}; changed the variable trimName to trimmedName; and called the method like this: trimmedName = BarChart.trimName(key);.

jdaguil · 2017-08-15T02:35:03Z

+        // Clear tooltip div created when inadvertently triggered during dropdown selection.
+        $( ".toolTip" ).remove();
+
+        let tooltip = d3.select("body").append("div").attr("class", "toolTip");


This is a little confusing since it is defined after it's used above. Can you move this up?

I moved 89-92 together -- .remove has to be called before we define the tooltip variable.

jdaguil · 2017-08-15T02:40:01Z

+                tooltip
+                    .style("left", d3.event.pageX - 50 + "px")
+                    .style("top", d3.event.pageY - 70 + "px")
+                    .style("display", "inline-block")


Is this needed since you set the display on mouseover?

You're right -- line 84 is redundant. Gone.

jdaguil · 2017-08-15T02:42:46Z

+                    .style("left", d3.event.pageX - 50 + "px")
+                    .style("top", d3.event.pageY - 70 + "px")
+                    .style("display", "inline-block")
+                    .html((d.name.replace('<', '&lt;') + ' (' + d.val + ')'));


This can be changed to:

.text((d.name + ' (' + d.val + ')'));

.html is used to insert html. You simply want to display text.

Thanks @jdaguil -- and I see the reference in the D3 3.x API.

jdaguil · 2017-08-15T02:54:09Z

+        chart.selectAll(".y.axis .tick")
+            .on("mouseover", function (d) { tooltip.style("display", "inline-block"); })
+            .on("mousemove", function (d) {
+                let id = d;


Same comment as above (renaming variable)

Both redundant variables are now gone.

jdaguil · 2017-08-15T02:54:31Z

+            .on("mouseover", function (d) { tooltip.style("display", "inline-block"); })
+            .on("mousemove", function (d) {
+                let id = d;
+                let displayValue = '';


Same comment as above (setting variable to an empty string)

jdaguil · 2017-08-15T02:54:48Z

+                tooltip
+                    .style("left", d3.event.pageX - 50 + "px")
+                    .style("top", d3.event.pageY - 70 + "px")
+                    .style("display", "inline-block")


Same comment as above

jdaguil · 2017-08-15T02:55:01Z

+                    .style("left", d3.event.pageX - 50 + "px")
+                    .style("top", d3.event.pageY - 70 + "px")
+                    .style("display", "inline-block")
+                    .html((d.replace('<', '&lt;') + displayValue));


Same comment as above

jdaguil · 2017-08-15T03:00:21Z

+                let id = d;
+                let displayValue = '';
+                let result = $.grep(summaryData, function (e) { return e.name === id; });
+                displayValue = (result.length === 1 ? ' (' + result[0].val + ')' : '');


Do we need this? Shouldn't there always be a result?

Signed-off-by: John M. Horan <johnmhoran@gmail.com>

* One note: retained this line of code because it’s needed, at least on Windows, but for some reason Jillian has not been able to reproduce on Mac: $( ".toolTip" ).remove(); Signed-off-by: John M. Horan <johnmhoran@gmail.com>

johnmhoran · 2017-08-16T18:04:55Z

@jdaguil Just pushed my refactoring/cleaning based on your comments. There's one item that may still be open.

You asked whether we need to retain this line of code: $( ".toolTip" ).remove();. As I noted in my reply to your comment, on my Windows 10 machine, without this code, a new toolTip div is generated each time the user selects a different attribute for display, like this:

I added this code to prevent the unwanted div growth, and placing it just before we define the tooltip variable limits the toolTip div to a single instance, like this:

What do you see on your Mac when you comment out the line of code?

jdaguil · 2017-08-16T19:46:34Z

@johnmhoran regarding #149 (comment) I misunderstood the issue in the code comment. I understand what you were trying to accomplish now, and it's fine.

jdaguil

Looking good! I've added a few small style nits

jdaguil · 2017-08-17T03:24:48Z

        let yAxis = d3.svg.axis()
            .scale(yScale)
+            // Limit label length to 50 characters plus ellipses.
+            .tickFormat(function(d) {


I think this can be rewritten as:

.tickFormat(BarChart.trimName)

jdaguil · 2017-08-17T03:27:27Z


        // Transform license count into array of objects with license name & count
        let chartData = $.map(count, function(val, key) {
+            let trimmedName = BarChart.trimName(key);


This is short enough now to where you can just move it into the object if you want:

trimmedName: BarChart.trimName(key);

Signed-off-by: John M. Horan <johnmhoran@gmail.com>

jdaguil · 2017-08-18T19:05:31Z

LGTM! 👍

johnmhoran assigned jdaguil Aug 7, 2017

Modify tooltip to move with cursor #141

b16ebec

Signed-off-by: John M. Horan <johnmhoran@gmail.com>

jdaguil reviewed Aug 8, 2017

View reviewed changes

Comment thread assets/css/main.css Outdated

}

/* Tooltip for Bar Chart */

.toolTip {

jdaguil Aug 8, 2017

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

nit: remove extra space.

jdaguil reviewed Aug 8, 2017

View reviewed changes

jdaguil suggested changes Aug 8, 2017

View reviewed changes

John M. Horan added 2 commits August 8, 2017 08:57

Merge branch 'develop' into branch 141 #141

a7bf7fb

* Update local branch with changes to 'develop'. Signed-off-by: John M. Horan <johnmhoran@gmail.com>

Add tooltips to y-axis labels #141

b499a62

* Bar chart labels now have tooltips with same content as corresponding bars. * Changed from truncated name to full name as appropriate. Signed-off-by: John M. Horan <johnmhoran@gmail.com>

jdaguil suggested changes Aug 15, 2017

View reviewed changes

John M. Horan added 2 commits August 15, 2017 13:49

Merge 'develop' into branch 141 #141

49250f9

Signed-off-by: John M. Horan <johnmhoran@gmail.com>

Refactor and clean code #141

a74d740

* One note: retained this line of code because it’s needed, at least on Windows, but for some reason Jillian has not been able to reproduce on Mac: $( ".toolTip" ).remove(); Signed-off-by: John M. Horan <johnmhoran@gmail.com>

jdaguil suggested changes Aug 17, 2017

View reviewed changes

Minor cleanup #141

5bae436

Signed-off-by: John M. Horan <johnmhoran@gmail.com>

jdaguil merged commit 7bb8123 into develop Aug 18, 2017

jdaguil deleted the 141-limit-bar-chart-label-width branch August 18, 2017 19:05

Uh oh!

Uh oh!

Conversation

johnmhoran commented Aug 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdaguil left a comment

Choose a reason for hiding this comment

Uh oh!

jdaguil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnmhoran commented Aug 16, 2017

Uh oh!

jdaguil commented Aug 16, 2017

Uh oh!

jdaguil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdaguil commented Aug 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants