custom colors for negative numbers #29

mrjones-plip · 2016-09-15T23:05:32Z

first stab at getting custom colors for negative numbers working per #28. Fix some bugs that should help, but not entirely fix, #27

. Fix some bugs that should help, but not entirely fix, #27

mrjones-plip · 2016-09-15T23:06:17Z

@melalj - i need to do some more testing - but any input is welcome!

mrjones-plip

impact of using Number() and removal of mystery code

mrjones-plip · 2016-09-16T05:20:35Z

src/components/GeoMap.js

@@ -434,32 +434,43 @@ export default class GeoMap {
      let minValue = attrValue.min;
      let maxValue = attrValue.max;

-      if (attrValue.min === 'minValue') {


@melalj - I couldn't tell what these lines did and didn't see any changes when removing them. If they're still needed - lemme know!

You should not remove these lines. Here's their application: https://github.com/Packet-Clearing-House/maptable#scaledvalue

great, thanks for confirming! d6a98e8 added them back.

mrjones-plip · 2016-09-16T05:32:07Z

src/components/GeoMap.js


-      const scaleDomain = d3.extent(dataset, d => d.rollupValue[attrKey]);
+      const scaleDomain = d3.extent(dataset, d => Number(d.rollupValue[attrKey]));


this, or the other call to Number() has made the column do an ASCII instead of a natural sort. The net result is that it orders the widget column in the negative.example.html example as "-10, -100 -110, -40..." :(

while I know in the columnsDetails you can define a dataParse function, i was just surprised that it defaulted to ASCII. We should do regression checks to ensure this doesn't change the existing default sort behavior.

Not sure why this would affect the sorting on the table.
We should maybe use dataParse to parse it as a number so that the sort for the table will work as well.

Hmm - we should check against master branch to see if it's a regression or not. I'll check and report back!

melalj · 2016-09-17T00:07:33Z

src/components/GeoMap.js

@@ -434,32 +434,43 @@ export default class GeoMap {
      let minValue = attrValue.min;
      let maxValue = attrValue.max;

-      if (attrValue.min === 'minValue') {


You should not remove these lines. Here's their application: https://github.com/Packet-Clearing-House/maptable#scaledvalue

melalj · 2016-09-17T00:08:03Z

src/components/GeoMap.js


      dataset.forEach(d => {
        let scaledValue;
-        if (!d.values.length || isNaN(d.rollupValue[attrKey])) {


There's any issue with tabs here :)

should be fixed in d6a98e8

melalj · 2016-09-17T00:09:34Z

src/components/GeoMap.js

-        maxValue = scaleDomain[1];
+
+      // check for negative color declorations
+      var useNegative = false;


You should use eslint with your IDE.
repalce var with let

melalj · 2016-09-17T00:10:40Z

src/components/GeoMap.js

+      if ((attrValue.maxNegative && !attrValue.minNegative) ||
+          (!attrValue.maxNegative && attrValue.minNegative)) {
+        throw new Error(`MapTable: maxNegative or minNegative undefined. Please declare both.`);
+      } else if (attrValue.maxNegative && attrValue.minNegative){


When you throw an error, it stop the JS execution.
So no need for a else, here

cools, removed else in d6a98e8.

melalj · 2016-09-17T00:11:19Z

src/components/GeoMap.js

+          (!attrValue.maxNegative && attrValue.minNegative)) {
+        throw new Error(`MapTable: maxNegative or minNegative undefined. Please declare both.`);
+      } else if (attrValue.maxNegative && attrValue.minNegative){
+        useNegative = true;


You can remove the declaration on L439 and use:
const useNegative = (attrValue.maxNegative && attrValue.minNegative);

lovely! added in d6a98e8

melalj · 2016-09-17T00:18:29Z

src/components/GeoMap.js


-      const scaleDomain = d3.extent(dataset, d => d.rollupValue[attrKey]);
+      const scaleDomain = d3.extent(dataset, d => Number(d.rollupValue[attrKey]));


Not sure why this would affect the sorting on the table.
We should maybe use dataParse to parse it as a number so that the sort for the table will work as well.

melalj · 2016-09-17T00:25:46Z

src/components/Legend.js

+          .attr('style', `stop-color:${this.map.options.countries.attr.fill.maxNegative};stop-opacity:1`);
+
+      legendGradient.append('stop')
+          .attr('offset', '49%')


Putting it as half is a wrong assumption if we have a minimum -20 and a maximum of 60. (for this use case the cut between the negative and positive is 25%). That's why the legend broke, because it's a scale only from 0 -> maxValue.

yes and no ;) yes, I agree that my new code should check to see what the smallest and largest numbers are and put the percentages in accordingly (not covered by d6a98e8, commit pending). However, no, the problem of negative numbers flying off the legend exists in master per #27. So i'll do the work to adjust what % we use for these stops, but it won't fix the legend.

ok - so legend still works as well as it did before with negative numbers flying off to the right, but now the zero position will show in a relative spot based on the min and max of the legend. check be8aa5d for details!

oh - hrm - the problem in #27 will be aggravated by this change. we'll need to update indiceChange() to pull it all together so it works.

mrjones-plip · 2016-09-17T21:44:05Z

@melalj - thanks so much for all the feedback! d6a98e8 should fix all of GeoMap.js, but I still need to fix Legend.js's display of of negative numbers in legend.

…dback per #28

mrjones-plip · 2016-09-19T19:30:01Z

Ohhh - the current branch is doing weird stuff with the legend now. If I remove the minNegative and maxNegative declarations in negative.example.html the legend works correctly. However, if I add them in, the color of the country isn't on the right scale. So I've given Mozambique the value of 11, but his color looks to be the value of about 30 or 40. I edited the exported SVG to better illustrate the error. I added a white "1" to denote the country value and a red arrow to show where it should be:

…ies section docs

first stab at getting custom colors for negative numbers working per #28

4ed1b84

. Fix some bugs that should help, but not entirely fix, #27

#28 flipped the colors, removed console.log debug

cedd15c

mrjones-plip commented Sep 16, 2016

View reviewed changes

melalj requested changes Sep 17, 2016

View reviewed changes

tidy up GeoMap.js #28 per feedback

d6a98e8

small refactor to Legend.js to have sliding scale 0 position, per fee…

be8aa5d

…dback per #28

Ths2-9Y-LqJt6 added 3 commits September 20, 2016 11:12

fix legend markers per #27 which also makes negative values work per #28

9b447ce

add docs for negative legend stuffs per #28, fix tabs on other countr…

929c298

…ies section docs

finish #28, fix #27, add release 1.2

5e4ced3

mrjones-plip merged commit d5e3d0f into master Sep 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

custom colors for negative numbers #29

custom colors for negative numbers #29

mrjones-plip commented Sep 15, 2016

mrjones-plip commented Sep 15, 2016

mrjones-plip left a comment

mrjones-plip Sep 16, 2016

melalj Sep 17, 2016

mrjones-plip Sep 17, 2016

mrjones-plip Sep 16, 2016

melalj Sep 17, 2016

mrjones-plip Sep 17, 2016

melalj Sep 17, 2016

melalj Sep 17, 2016

mrjones-plip Sep 17, 2016

melalj Sep 17, 2016

mrjones-plip Sep 17, 2016

melalj Sep 17, 2016

mrjones-plip Sep 17, 2016

melalj Sep 17, 2016

mrjones-plip Sep 17, 2016

melalj Sep 17, 2016

melalj Sep 17, 2016

mrjones-plip Sep 17, 2016

mrjones-plip Sep 19, 2016

mrjones-plip Sep 19, 2016

mrjones-plip commented Sep 17, 2016

mrjones-plip commented Sep 19, 2016


		const scaleDomain = d3.extent(dataset, d => d.rollupValue[attrKey]);
		const scaleDomain = d3.extent(dataset, d => Number(d.rollupValue[attrKey]));

custom colors for negative numbers #29

custom colors for negative numbers #29

Conversation

mrjones-plip commented Sep 15, 2016

mrjones-plip commented Sep 15, 2016

mrjones-plip left a 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

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

mrjones-plip commented Sep 17, 2016

mrjones-plip commented Sep 19, 2016