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

Fix elasticY with evadeDomainFilter #1367

Closed
wants to merge 3 commits into from

Conversation

dahlbyk
Copy link
Contributor

@dahlbyk dahlbyk commented Feb 28, 2018

Fixes that evadeDomainFilter, introduced by 77d22ba for #949, doesn't work with elasticY(true). The fix requires access to the domain-filtered values separate from all values needed to render the chart.

Bonus fix: x().domain() transitions from [0, _] to [x, _], as in the bar transitions demo with "right" selected, will now rescale() correctly.

@gordonwoodhull
Copy link
Contributor

Thank you for all these excellent contributions, and sorry for my slow response.

I was in a car accident last week and broke my collarbone, so I haven't been able to spend much time at the computer.

I am very excited to review your three PRs when I'm able, but it won't be for at least another week.

@dahlbyk
Copy link
Contributor Author

dahlbyk commented Mar 5, 2018

I was in a car accident last week and broke my collarbone, so I haven't been able to spend much time at the computer.

Oh my, so sorry to hear that. Hope you have a quick recovery!

@dahlbyk dahlbyk changed the base branch from master to 3.0 April 9, 2018 06:44
@dahlbyk dahlbyk force-pushed the elasticY-evadeDomainFilter branch from 004647d to c221d15 Compare April 9, 2018 06:45
@dahlbyk
Copy link
Contributor Author

dahlbyk commented Apr 9, 2018

Rebased onto 3.0.

I'll leave the 2.x version at da3c437...NewBoCo:elasticY-evadeDomainFilter-2.x in case anyone needs it.

@gordonwoodhull
Copy link
Contributor

Ok, I was about to look at d3.stack #1375 so I figured I should merge this first.

Thanks @dahlbyk, this makes a lot of sense. I added a simple test, too - turns out we already covered this situation but didn't test it.

Let's fix those line endings too.

I'm dropping the fix to compareDomains because @kum-deepak has a more comprehensive fix in the pipeline in #1402.

This will go into 3.0 alpha 7.

gordonwoodhull added a commit that referenced this pull request Apr 13, 2018
@dahlbyk dahlbyk deleted the elasticY-evadeDomainFilter branch April 14, 2018 06:01
@dahlbyk
Copy link
Contributor Author

dahlbyk commented Apr 14, 2018

#1391 (comment): I found it a bit confusing but maybe I didn't stare at it long enough.

To clarify what I'm trying to fix, with elasticY(true) added to the bar-transition demo:

With evadeDomainFilter(false)

Note bars outside the filtered domain transition up/down.
elasticy-evadedomainfilter-false

With evadeDomainFilter(true), broken

Bars outside the domain filter now slide in/out (the point of evadeDomainFilter), but the y-axis is not elastic because flattenStack always looks at values for the full domain.
elasticy-broken

With evadeDomainFilter(true), fixed

Now y-axis is elastic because flattenStack considers values for the current domain (saved in layer.domainValues) while layer.values keeps all values to evades the domain filter.
elasticy-fixed

@gordonwoodhull
Copy link
Contributor

Thanks for the demo! That's very helpful for documenting the change going forward.

Yesterday when I looked at this PR again, I understood it, and this fix is already on the 3.0 branch along with a test (linked above).

I haven't tagged it yet. Hoping to include it in alpha 7 with part of #1402 today.

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