Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Choropleth keep d3.geoAlbers the default projection but deprecate it #1382

Closed
wants to merge 3 commits into from
Closed

Choropleth keep d3.geoAlbers the default projection but deprecate it #1382

wants to merge 3 commits into from

Conversation

kum-deepak
Copy link
Collaborator

Reference #1379

@kum-deepak
Copy link
Collaborator Author

@gordonwoodhull The code seems to keep a flag to apply the projection again if it changes. I have a hunch that _projectionFlag can be removed.

If consider the general convention of dc, in case of any change other than data, render must be called. I think that it can be cleaned up.

I will create an independent commit so that you can reject that part without affecting the core work.

@kum-deepak
Copy link
Collaborator Author

I am updating the VC example where in 6 seconds the projection is changed and the chart re-rendered. I am pushing that as a separate commit, the last commit should not be merged, it is only for testing.

- Updated example
- Deactivated cross browser tests, these were failing for pull requests
@kum-deepak
Copy link
Collaborator Author

I have disabled cross browser tests, those were always failing for me :)

Maybe we switch back to old scheme of invoking cross browser testing.

@gordonwoodhull
Copy link
Contributor

Yeah, I tried to find earlier discussion, but certainly disabling saucelabs for pull requests was intentional (16e6863). So I'm reverting eff5bf9.

I wouldn't say that there is a convention that redraw is only for data changes. Rather I would say that redraw works for whatever people have debugged it for. So I think I'll keep _projectionFlag. No one ever said simplicity was one of dc.js's virtues. 😉

@gordonwoodhull
Copy link
Contributor

I took the first commit and tested using your VC example test, only with redraw not render, and with a more dramatic projection change (Mercator). Works great!

@gordonwoodhull
Copy link
Contributor

Thanks @kum-deepak!

@kum-deepak
Copy link
Collaborator Author

Thanks @gordonwoodhull!

I am intending to devote time on this project for reasonable time. We are relying on this library heavily and will like it to do even more. To that effect, I will participate in cleaning up as much as possible.

I have realized that the code has been written over many years and has varying code quality across files.

Should we open a separate task for discussion - to cover what someone can expect out of redraw vs render. This will simplify the library and make life easier for users.

@gordonwoodhull
Copy link
Contributor

Sure, it's worth discussing.

In general the functional difference is that redraw will transition from the old state to the new state, but render starts from a blank SVG, so there is usually a blink.

Zooming is one example of something besides the data changing, and although the choropleth doesn't have transitions, perhaps it could morph from one projection to another.

@kum-deepak
Copy link
Collaborator Author

It makes sense. :) There is so much I do not know about this library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants