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

Support gaps in data (null, NaN, etc) #46

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

motiz88
Copy link
Contributor

@motiz88 motiz88 commented Apr 12, 2016

I'm submitting this PR to gauge interest in the completed feature and get some feedback before I go ahead and implement it all.
The idea is to graphically represent data with in-band nulls, NaNs or infinite values by drawing the valid (finite numeric) segments separately from one another.

@borisyankov
Copy link
Owner

That is a really good idea. 👍

@motiz88
Copy link
Contributor Author

motiz88 commented Apr 13, 2016

Whew! OK, I think this thing is quite complete. Any thoughts?

As for me, I still wonder about the gaps prop. Right now it's not treated very consistently in the code and I'm thinking about removing it entirely. So the equivalent of gaps={true} would be the default and only behavior. I'd appreciate your opinion here.

@motiz88 motiz88 changed the title WIP: Support gaps in data (null, NaN, etc) Support gaps in data (null, NaN, etc) Apr 13, 2016
@borisyankov
Copy link
Owner

I agree, this being the default options, sounds like a good idea.

@motiz88
Copy link
Contributor Author

motiz88 commented Apr 14, 2016

I implemented this and one existing test was broken - this one. I'll rebase this on master in a minute, so we can use Travis here again.

It's your call, @borisyankov. As a user, I'm comfortable with this change, but I'm just one data point. Having this on by default should probably be semver-major to avoid breaking others' code.

@jarib
Copy link

jarib commented Sep 3, 2016

@borisyankov Any thoughts on getting this merged? I could use it :)

@timelyportfolio
Copy link
Contributor

@motiz88 any interest in getting this current with master? I am trying to pull this my branch but am late to the party and unsure of status. Thanks.

@borisyankov
Copy link
Owner

@motiz88 Has been a while. I am committed to merging it into master.

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.

4 participants