Color legend widget #731

matthewma7 · 2017-08-18T02:55:55Z

Implementation of color legend widget.

Support both discrete and continuous scale.
Scale calculation is based on d3 scale.
Discrete color scale supports linear log, sqrt, pow, quantile, ordinal.
Continuous color supports linear log, sqrt, pow. Continuous color supports two color transition.

This is is WIP PR, will test use this widget in a project first.

… Adds container style;

…nt, don't add unnecessary precision;

aashish24 · 2017-08-18T15:12:42Z

this is looking awesome @MatthewMa

…ng and removing categories; other improvements;

matthewma7 · 2017-08-31T14:21:16Z

This is ready for review

manthey · 2017-09-01T17:56:35Z

src/ui/colorLegendWidget.js

+var registerWidget = require('../registry').registerWidget;
+
+/**
+ * @typedef {object} [geo.gui.colorLegendWidget.category]


You don't want the [] around the typedef.

manthey · 2017-09-01T17:56:40Z

src/ui/colorLegendWidget.js

+ * @alias geo.gui.colorLegendWidget
+ * @extends geo.gui.domWidget
+ * @param {object} [arg] Widget options.
+ * @param {object} [args.position] Position setting relatively to the map container.


This should be arg.position not args.position (and similar on lines below).

manthey · 2017-09-01T18:02:51Z

src/ui/colorLegendWidget.js

+/**
+ * @typedef {object} [geo.gui.colorLegendWidget.category]
+ * @property {string} name The text label of the legend.
+ * @property {string} type The type of the legend, either discrete or continuous.


Should there be a default value? Perhaps default this to continuous and and the scale to linear.

manthey · 2017-09-01T18:03:31Z

src/ui/colorLegendWidget.js

+ * @property {string} scale The scale of the legend. For discrete type,
+ * linear, log, sqrt, pow, ordinal, and quantile is supported.
+ * For continuous type, linear, log, sqrt, and pow is supported.
+ * @property {number[]|string[]} domain Only for ordinal scale legend, string values are acceptable. For ordinal legend, the number in the domain array should be the same number of colors. For quantile scale legend, the domain should be an array of all values. For other scales, the domain needs to be an array of two number for marking the upper bound and lower bound.


Elsewhere we've wrapped comments at 80 columns (and in general, @aashish24 at least has preferred wrapped lines).

manthey · 2017-09-01T18:08:12Z

src/ui/colorLegendWidget.js

+ * linear, log, sqrt, pow, ordinal, and quantile is supported.
+ * For continuous type, linear, log, sqrt, and pow is supported.
+ * @property {number[]|string[]} domain Only for ordinal scale legend, string values are acceptable. For ordinal legend, the number in the domain array should be the same number of colors. For quantile scale legend, the domain should be an array of all values. For other scales, the domain needs to be an array of two number for marking the upper bound and lower bound.
+ * @property {string[]} colors The colors of the legend. All valid svg color can be used. For discrete type, multiple values are accepted. For continuous type, an array of two values is supported.


Perhaps add a note that this corresponds to the d3.scale.range parameter. Also, allowing only svg colors is more restrictive then how colors are supported elsewhere. We should pass the colors through geo.util.convertColorToHex with allowAlpha set to true. This would ensure a consistent color definition in geojs. In which, the parameter type can be changed to geo.geoColor.

manthey · 2017-09-01T18:46:41Z

src/ui/colorLegendWidget.js

+      m_this._renderAxis(svg, axis);
+    }
+
+    function createAxis(axisScale) {


Please document.

manthey · 2017-09-01T18:48:21Z

src/ui/colorLegendWidget.js

+ * For continuous type, linear, log, sqrt, and pow is supported.
+ * @property {number[]|string[]} domain Only for ordinal scale legend, string values are acceptable. For ordinal legend, the number in the domain array should be the same number of colors. For quantile scale legend, the domain should be an array of all values. For other scales, the domain needs to be an array of two number for marking the upper bound and lower bound.
+ * @property {string[]} colors The colors of the legend. All valid svg color can be used. For discrete type, multiple values are accepted. For continuous type, an array of two values is supported.
+ */


I see some other properties that are optional on categories (base and exponent, I think). They should be added to the typedef as well.

manthey · 2017-09-01T18:50:26Z

tests/cases/colorLegend.js

+  it('Create basic color legend widget', function (done) {
+    expect($(container).find('.legend').length).toBe(1);
+    done();
+  });


For synchronous tests, you don't need to use the done parameter -- just use function () and don't call done().

manthey · 2017-09-01T18:55:46Z

tests/cases/colorLegend.js

+    $(legends[1]).find('svg>rect')[0].dispatchEvent(mouseout);
+    done();
+  });
+});


The code coverage misses a few things: calling categories() without a parameter and testing that you get the values you set; quantile rendering discrete colors; testing against an unsupported scale; hiding the popup (should that have been triggered by a mouse movement?); mouseenter and mouseleave; a scale with zero categories.

Wow, how do you notice such small misses, thank you for such careful review.
I think I tested popup mouseout event, did the coverage says otherwise? Do you know how to see the line by line code coverage for the colorLegend file?

https://codecov.io/gh/OpenGeoscience/geojs/src/2d_legend/src/ui/colorLegendWidget.js

Also, if you run the tests with coverage on locally, it will dump an html coverage report that you can browse.

@jbeezley, Thank you! That's really helpful.

manthey · 2017-09-01T19:10:00Z

examples/color-legend/example.json

+{
+  "path": "color-legend",
+  "title": "Color legends",
+  "exampleCss": ["main.css"],


For examples with empty css files, we really should set exampleCss: [] and shouldn't include a blank css file.

matthewma7 · 2017-09-05T16:58:28Z

@manthey, Thank you very much for your detailed previous reviews. They totally make sense.
I have made corresponding changes.
This is ready for further review.

(I don't know why codecov indicates two line is not covered, I have a expect() that tests the results from that two lines)

manthey · 2017-09-05T18:27:19Z

src/ui/colorLegendWidget.js

+ * container.
+ * @param {geo.gui.colorLegendWidget.category[]} [arg.categories] An array
+ * of category definitions for the initial color legends
+ * @param {number} [arg.width] The pixel width of the wiget in number. Default is 300px.


jsdoc lets you specify defaults as part of an optional parameter:

@param {number} [arg.width=300] The width of the widget in pixels.

Currently height and margins are hard-coded too. I think that rather than just specifying width and ticks, we should allow width, height, horizontal_margin (or perhaps hmargin), vertical_margin, and ticks. I think that it would be more intuitive if width and height were the size of the color scale, and the horizontal_margin (or whatever we call it) would be the spacing at either end and the vertical_margin would be the spacing at the bottom for the ticks and labels.

manthey · 2017-09-05T18:37:37Z

src/ui/colorLegendWidget.js


    if (arg.categories) {
      this.categories(arg.categories);
    }
  };

-  // clear the DOM container and create legends
+  /**
+   * clear the DOM container and create legends


This should fail the jsdoc function description template rules, which require a capital letter and terminal punctuation. Please check for other jsdoc errors on this file (they are reported as warnings).

manthey · 2017-09-05T18:39:51Z

src/ui/colorLegendWidget.js

  };

-  // Draw an individual discrete type legend
+  this._prepareCategories = function (categories) {


Please comment the function.

manthey · 2017-09-05T18:58:58Z

src/ui/colorLegendWidget.js

@@ -270,8 +297,9 @@ var colorLegendWidget = function (arg) {
    if (category.scale === 'pow' && category.exponent) {
      axisScale.exponent(category.exponent);
    }
-    var randomString = Math.random().toString(36).substring(5);
-    var precision = Math.max.apply(null, category.domain.map(function (number) { return getPrecision(number); }));
+    var randomString = util.randomString(5);


I should have noticed before. We already have a function in d3/uniqueID for this purpose, so we should use that.

manthey · 2017-09-05T19:03:05Z

src/ui/widget.js

+ * @property {string|number} bottom The position to the bottom of the container.
+ * Value is used similarly to the top property.
+ * @property {string|number} left The position to the left of the container.
+ * Value is used similarly to the top property.


All of these properties are optional. Further, we should add:

@property {*} [...] Additional css properties that affect position are allowed. See the css specification for details.

manthey · 2017-09-18T13:19:45Z

With PR #733, you can remove examples/color-legend/index.pug and examples/color-legend/main.css, and remove the lines that have path and example.css from examples/color-legend/example.json.

matthewma7 · 2017-09-18T21:47:47Z

OMG, I finally fixed the build. @manthey, could you take a quick look?

manthey · 2017-09-19T13:10:35Z

src/ui/colorLegendWidget.js

+   * Formatter of number that tries to maximize the precision
+   * while making the output shorter.
+   * @param {number} number to be formatted
+   * @param {precision} precision number of decimal precision will be tried to be kept


I think the data type should just be number. I'd recommend changing this to @param {number} precision Maximum number of decimal places that are kept..

manthey · 2017-09-19T13:27:39Z

src/ui/colorLegendWidget.js

+  return this;
+};
+
+function getPrecision(a) {


This function needs to be commented (while you at it, _hidePopup, above, needs it too).

manthey · 2017-09-19T13:29:19Z

src/ui/colorLegendWidget.js

+function getPrecision(a) {
+  if (!isFinite(a)) return 0;
+  var e = 1, p = 0;
+  while (Math.round(a * e) / e !== a) { e *= 10; p++; }


If you pass a value with a magnitude less than 1 / Number.MAX_VALUE, the process can get stuck in an infinite loop (if Number.MIN_VALUE is less than 1 / Number.MAX_VALUE , which is true in my version of Chrome). Perhaps we should return if isFinite(a * e) is false.

Chrismarsh · 2017-09-30T19:16:38Z

I realize this is closed, however I have a question regarding how to use this. When I add a contour plot as well as one of these legends, the colouring of the contour plot doesn't match the legend's colour. Is this to be expected?

manthey · 2017-10-02T12:12:40Z

Are you using the same colors for the contour plot and the legend? They can be specified independently; there is nothing that forces them to be the same, though you should be able to use the same colors for both.

If you have a code snippet, we can advise further.

matthewma7 added 9 commits August 15, 2017 16:45

initial proof of concept

918efa1

initial functional implementation

921ea7e

Reverts some unintential changes;

f6f4466

Initial implementation of mouseover popup; Other improvements;

84a8d93

Limits number of axis ticks for all type of legend; Adds legend name;…

d45d2f1

… Adds container style;

Supports pass in categories in beginning; precision display improveme…

ced0ec2

…nt, don't add unnecessary precision;

Adds support for quantile scale

c903e4a

Improves popup positioning;

3931db6

Fixes eslint errors;

c21553d

matthewma7 added 3 commits August 18, 2017 15:29

Rename from legend2DWidget to colorLegendWidget; add methods for addi…

fd4b166

…ng and removing categories; other improvements;

Adds legend tick limit to ordinal scale as well.

1d78b45

Adds unit test

1fec30d

matthewma7 force-pushed the 2d_legend branch from bec9cb7 to 1fec30d Compare August 25, 2017 22:05

Adds JSDocs

5b1811f

matthewma7 force-pushed the 2d_legend branch from b7caa62 to 5b1811f Compare August 28, 2017 17:08

Merge branch 'master' into 2d_legend

488118a

matthewma7 changed the title ~~[WIP] Color legend widget~~ Color legend widget Aug 31, 2017

manthey requested changes Sep 1, 2017

View reviewed changes

matthewma7 force-pushed the 2d_legend branch 3 times, most recently from bb2c552 to 9f0e27f Compare September 5, 2017 16:39

Updates jsdocs; refactors codes based on review.

8e9e5b8

matthewma7 force-pushed the 2d_legend branch from 9f0e27f to 8e9e5b8 Compare September 5, 2017 16:42

manthey requested changes Sep 5, 2017

View reviewed changes

Further jsdocs changes

dba0a88

aashish24 mentioned this pull request Sep 13, 2017

Provide a color legend (rectangular) for plots showing value to color mapping #714

Closed

matthewma7 added 2 commits September 18, 2017 10:40

Merge branch 'master' into 2d_legend

d0e663b

Conforms with latest example framework

03cb58e

matthewma7 force-pushed the 2d_legend branch from bed3844 to 63bee9a Compare September 18, 2017 19:02

Updates test to support all three test browsers

b8f5c71

matthewma7 force-pushed the 2d_legend branch from 63bee9a to b8f5c71 Compare September 18, 2017 20:21

continously tries to make the test pass;

8bbf1d7

manthey requested changes Sep 19, 2017

View reviewed changes

Some minor fixes based on review

74759cd

manthey approved these changes Sep 19, 2017

View reviewed changes

matthewma7 merged commit cb4ea54 into master Sep 19, 2017

matthewma7 deleted the 2d_legend branch September 19, 2017 18:49

manthey mentioned this pull request Feb 7, 2018

Provide multi-color legend for numeric values #734

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Color legend widget #731

Color legend widget #731

matthewma7 commented Aug 18, 2017

aashish24 commented Aug 18, 2017

matthewma7 commented Aug 31, 2017

manthey Sep 1, 2017

manthey Sep 1, 2017

manthey Sep 1, 2017

manthey Sep 1, 2017

manthey Sep 1, 2017

manthey Sep 1, 2017

manthey Sep 1, 2017

manthey Sep 1, 2017

manthey Sep 1, 2017

matthewma7 Sep 4, 2017

jbeezley Sep 4, 2017

matthewma7 Sep 4, 2017

manthey Sep 1, 2017

matthewma7 commented Sep 5, 2017

manthey Sep 5, 2017

manthey Sep 5, 2017

manthey Sep 5, 2017

manthey Sep 5, 2017

manthey Sep 5, 2017

manthey Sep 5, 2017

manthey commented Sep 18, 2017

matthewma7 commented Sep 18, 2017

manthey Sep 19, 2017

manthey Sep 19, 2017

manthey Sep 19, 2017

Chrismarsh commented Sep 30, 2017

manthey commented Oct 2, 2017

Color legend widget #731

Color legend widget #731

Conversation

matthewma7 commented Aug 18, 2017

aashish24 commented Aug 18, 2017

matthewma7 commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewma7 commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manthey commented Sep 18, 2017

matthewma7 commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chrismarsh commented Sep 30, 2017

manthey commented Oct 2, 2017