Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

D3v5 compatibility #1394

Closed
wants to merge 7 commits into from
Closed

D3v5 compatibility #1394

wants to merge 7 commits into from

Conversation

kum-deepak
Copy link
Collaborator

Very few changes and all test cases pass. Good thing is that change for us can be done in backward compatible way.

Examples will need changes because they now use Promises. Once changed it works. As an example I have changed the stocks example.

We can finish it after dc v3 release.

@gordonwoodhull
Copy link
Contributor

Awesome!

I see no need to complicate things by releasing two versions at once. I think that we can update all the examples to v5 and have dc.js v3 compatible with both d3v4 and v5.

On a personal note, I don't generally work weekends. Thanks for all the hard work - I look forward to merging your stuff tomorrow!

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Apr 2, 2018

As for the colors, there are good reasons why schemeCategory20c was removed. So we should switch probably to schemeCategory10c or a colorbrewer scheme e.g. as suggested in #831.

This means fixing a lot of tests, documenting the change pointing to the rationale in d3v5, and offering the workaround used here in the documentation. IMO we should not provide schemeCategory20c in the library because it is misleading.

@kum-deepak
Copy link
Collaborator Author

Fully agree!

Once other PRs have been merged, I will go ahead and make the changes.

Alternatively we can introduce a global switch to keep current behavior with a warning. In 3.1 we can make full switch.

BTW there are not many (less than 10) test cases that we would need to fix.

@gordonwoodhull
Copy link
Contributor

The color brewer colors are dramatically different but much more clear, so I agree we should probably do backward compatibility with a warning.

We don't currently have any global flags, but that's an interesting idea. Maybe it would be worth introducing a special facility under dc just for this, say dc.compatFlags, since we have so many things we want to fix while allowing backward compatibility.

In this case, there could be a flag, say brewerCategoricalColors, which defaults to false. The first time we need defaulted categorical colors, because the chart doesn't have any colors set, we check this flag and if it's still false, we use the copied colors from above and issue a warning that says we're moving to brewer and mentions schemeCategory10c, which is close to the old default but obviously has less colors.

If it's true, we use the color brewer colors with no warning.

@kum-deepak
Copy link
Collaborator Author

I will do a commit implementing it schematically, we can then progress from there.

@kum-deepak
Copy link
Collaborator Author

I have reached a milestone on this. Key changes:

  • compat code
  • d3.csv/d3.json changes
  • play-ground.html got missed during D3v4 round, works similar to current version
  • I realized two of resizing examples were failing, fixed now
  • The current version resizing examples don't work (I had done some fixes during D3v4 work)
  • Some places I have changed == to === and != to !==

These are mechanical changes, at this stage we can merge. The other work for flag and warning message etc., can be done a separate PR.

In addition, I will work onto to run tests on both D3v4 and D3v5 (as a separate PR)

@gordonwoodhull
Copy link
Contributor

Any reason I shouldn't merge this now?

@kum-deepak
Copy link
Collaborator Author

I think we can merge this. Currently I am not aware of any issues in this regard.

@gordonwoodhull
Copy link
Contributor

Great, thanks @kum-deepak! I filed #1403 for removing schemeCategory20c.

Released as 3.0.0-alpha.5 (but still not to npm).

@kum-deepak kum-deepak deleted the d3v5 branch April 22, 2018 11:13
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants