Remove schemeCategory20c #1409

kum-deepak · 2018-04-15T07:43:58Z

This PR contains following:

Concept of dc.config, which will be a single instance object. It currently holds one setting and one variable. Please see if you are ok with the concept.
For backward compatibility, following needs to be added before creating charts:

    dc.config.useV2CompatColorScheme(true);

Concept of standard colors in specs. This will help us in minimizing breakage when making global changes in color palette.
A special matcher for color which covers more cases than a reg ex.
Enabled back index.html generation in specs folder, this allows to run test cases interactively by going to http://localhost:8888/spec/ - useful for interactive debugging.

Charts work but they do look different :)

kum-deepak · 2018-04-15T08:06:32Z

If you like the concept of dc.config, it can be used for more things. One immediate usage may be to keep the default ordinal colors (which currently lie in color mixin).

gordonwoodhull

Really clean implementation, and overall this helps clarify a lot of things.

I'll be interested to hear if you think it should be defaulted to the new colors, or if that was a miscommunication. Open to any ideas here.

The other big question is if we should offer a permanent way to set the default colors - might be helpful.

The config object and the matcher are great infrastructure improvements.

gordonwoodhull · 2018-04-15T15:06:52Z

spec/helpers/standard-colors.js

+    '#e41a1c', '#377eb8', '#4daf4a',
+    '#984ea3', '#ff7f00', '#ffff33',
+    '#a65628', '#f781bf', '#999999'
+];


Why copy d3.schemeSet1 here? It's not going to change in d3 and if it did, it would still break all our tests.

Actually I like the idea of defaultColors in config, once we move there, we can use that to match, this variable would not be needed at all.

gordonwoodhull · 2018-04-15T15:09:29Z

src/config.js

+dc.config = (function () {
+    var _config = {};
+
+    var _useV2CompatColorScheme = false;


I think we should default to true and issue the warning. This way people get backward compatibility by default, to ease their transition into the new world, but they are also informed that it's going away.

gordonwoodhull · 2018-04-15T15:09:52Z

src/config.js

+
+    /**
+     * Set this flag to use DCv2's color scheme (d3.schemeCategory20c).
+     * Please note this color scheme has been deprecated by D3


It's been removed in d3v5.

Thanks, will change!

gordonwoodhull · 2018-04-15T15:15:31Z

src/config.js

+     * Set this flag to use DCv2's color scheme (d3.schemeCategory20c).
+     * Please note this color scheme has been deprecated by D3
+     * (https://github.com/d3/d3/blob/master/CHANGES.md#changes-in-d3-50) as well as DC.
+     * @method useV2CompatColorScheme


Let's add an example of what you should do if you want to keep the old colors and not get the warning. Namely, you'd copy _schemeCategory20c and apply it to each chart.

Alternately, since we are adding a level of indirection here, we could rename compatColors to defaultColors and make it settable and permanent, so that users could automatically have the same color set for all charts. That might be a good idea if it turns out that schemeSet1 was the wrong choice.

If we allow setting defaultColors, we need to add in the API docs. In addition we may create an example to show how to use their color scheme.

gordonwoodhull · 2018-04-15T15:26:14Z

spec/color-spec.js

@@ -48,7 +50,8 @@ describe('dc.colorMixin', function () {
        });

        it('default', function () {
-            expect(colorTest(chart, domain, test)).toMatchColors(['#9ecae1','#3182bd','#c6dbef','#6baed6','#e6550d','#3182bd']);
+            expect(colorTest(chart, domain, test)).toMatchColors([standardColors[2], standardColors[0],
+                standardColors[3], standardColors[1], standardColors[4], standardColors[0]]);


This test is really opaque - is there any way to make it clear what it proves? I can't figure it out, it's not numerical or alphabetical order is it?

(We have this problem in a lot of our tests, and it's unfortunate because it means people will "fix the tests" instead of understanding why they failed.)

I have realized what it does, I will refactor it is a way that it does not seem so opaque :)

gordonwoodhull · 2018-04-15T15:26:20Z

spec/color-spec.js

-            expect(colorTest(chart, domain)).toMatchColors(['#3182bd','#6baed6','#9ecae1','#c6dbef','#e6550d']);
+            expect(colorTest(chart, domain))
+                .toMatchColors([standardColors[0], standardColors[1],
+                    standardColors[2], standardColors[3], standardColors[4]]);


standardColors.slice(0, 5)

kum-deepak · 2018-04-15T15:45:51Z

I will make one more iteration and then we can have one more round of discussion. I want to get it right :)

kum-deepak · 2018-04-16T06:36:38Z

Moved to concept of dc.config.defaultColors. Refactored few test cases for readability and resilience. Added additional notes for opaque-ish test cases.

If we are happy with the approach, I will add the warning message.

For one time warning messages, I want to create one helper method in dc.logger, which will ensure one message is shown only once.

gordonwoodhull · 2018-04-17T01:56:38Z

Huh, until now I thought we need the flag as well as defaultColors.

We do need to warn when defaultColors is not set, but maybe it's sufficient to test that it is not _schemeCategory20c - is that what you are thinking?

kum-deepak · 2018-04-17T01:58:30Z

Yes, that indeed was my thinking - this led me to the need of warnOnce (in dc.logger). Simplifies the code :)

…//localhost:8888/spec/

…t of `dc.config`. Made matching of colors in specs linked to variables to make cases less fragile.

…dability and resilience.

kum-deepak · 2018-04-17T02:53:55Z

Took a stab at adding to Change log as well. :) English is my 3rd language, so you will need to review these text as well.

gordonwoodhull · 2018-04-17T02:58:04Z

Sure, I don't mind doing copy editing. It's really helpful if you put something there so it doesn't get lost.

kum-deepak · 2018-04-17T03:39:51Z

Completed my current round of work on this one.

gordonwoodhull · 2018-04-18T23:37:52Z

Merged in 3.0 alpha 10

kum-deepak mentioned this pull request Apr 15, 2018

remove schemeCategory20c in a backward-compatible way #1403

Closed

gordonwoodhull reviewed Apr 15, 2018

View reviewed changes

kum-deepak mentioned this pull request Apr 16, 2018

dc.logger enhacements #1411

Closed

kum-deepak added 4 commits April 17, 2018 07:39

Enabled jasmine:specs:build to facilitate debugging by going to http:…

b7209f6

…//localhost:8888/spec/

Replaced schemeCategory20c in backward compatible way. Created concep…

6b841b2

…t of `dc.config`. Made matching of colors in specs linked to variables to make cases less fragile.

Concept of dc.config.defaultColors. Refactored few test cases for rea…

3212949

…dability and resilience.

Deprecation warning

5dfb1cf

kum-deepak mentioned this pull request Apr 17, 2018

General discussion and update #1391

Closed

gordonwoodhull closed this Apr 18, 2018

kum-deepak deleted the color-palette branch April 28, 2018 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove schemeCategory20c #1409

Remove schemeCategory20c #1409

kum-deepak commented Apr 15, 2018

kum-deepak commented Apr 15, 2018

gordonwoodhull left a comment

gordonwoodhull Apr 15, 2018

kum-deepak Apr 15, 2018

gordonwoodhull Apr 15, 2018

kum-deepak Apr 15, 2018

gordonwoodhull Apr 15, 2018

kum-deepak Apr 15, 2018

gordonwoodhull Apr 15, 2018

kum-deepak Apr 15, 2018

gordonwoodhull Apr 15, 2018

kum-deepak Apr 15, 2018

gordonwoodhull Apr 15, 2018

kum-deepak Apr 15, 2018

kum-deepak commented Apr 15, 2018

kum-deepak commented Apr 16, 2018

gordonwoodhull commented Apr 17, 2018

kum-deepak commented Apr 17, 2018

kum-deepak commented Apr 17, 2018

gordonwoodhull commented Apr 17, 2018

kum-deepak commented Apr 17, 2018

gordonwoodhull commented Apr 18, 2018

Remove schemeCategory20c #1409

Remove schemeCategory20c #1409

Conversation

kum-deepak commented Apr 15, 2018

kum-deepak commented Apr 15, 2018

gordonwoodhull 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

kum-deepak commented Apr 15, 2018

kum-deepak commented Apr 16, 2018

gordonwoodhull commented Apr 17, 2018

kum-deepak commented Apr 17, 2018

kum-deepak commented Apr 17, 2018

gordonwoodhull commented Apr 17, 2018

kum-deepak commented Apr 17, 2018

gordonwoodhull commented Apr 18, 2018