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

Moved .exit before .enter, should fix #1460 #1463

Closed
wants to merge 1 commit into from
Closed

Moved .exit before .enter, should fix #1460 #1463

wants to merge 1 commit into from

Conversation

kum-deepak
Copy link
Collaborator

Possible fix of #1460.

@kum-deepak
Copy link
Collaborator Author

@Frozenlock please test it out, see if it resolves the issue.

@Frozenlock
Copy link
Contributor

Yup!

@kum-deepak
Copy link
Collaborator Author

@gordonwoodhull for your feedback.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Jul 7, 2018

I'm willing to merge this, but as I asked in #1460 (comment), I would still like to know why this works.

If selection.exit() has no side effects, and transition.remove() is delayed by the transitionDuration, how come this has any effect?

Deepening the mystery, Mike's General Update Pattern series of blocks have .remove() after enter and update... up until the last example, where transitions are introduced. So I think there may be something about the asynchronous nature of transitions which make this necessary.

@kum-deepak
Copy link
Collaborator Author

@gordonwoodhull it indeed is a mystery to me that how it works. If we consider the original code and see http://localhost:8888/web/examples/scatter-brushing.html, symbols do get removed. So, in general there is no issue in .exit and .remove.

@Frozenlock, I could not find a link to a fiddle to check the problem. Please share the link (preferably the filter one), I will dig deeper into it.

@Frozenlock
Copy link
Contributor

@kum-deepak My apologies, but the gif is from a pretty complex setup in Clojurescript.
In fact I don't program in javascript. I can read, tweak a few things, but that's about it.

The simplest way to test this is probably to have a button with which you can remove an entry in the chart's group. @gordonwoodhull will probably know more about what is going on, as he correctly guessed I was using fake groups.

@gordonwoodhull
Copy link
Contributor

Okay I'll try to test this.

The scatter-brushing escape doesn't exhibit the problem because no bins are actually removed - the values just go to zero and the chart reacts by setting the size to zero based on the existenceAccessor.

This is what you get with straight crossfilter, which is probably why we didn't have any Jasmine tests for it.

@gordonwoodhull
Copy link
Contributor

Ah, okay, the answer is really trivial. I was totally overthinking it. 😊

It's just that using this coding pattern we replace the symbols variable. The result selection of .merge(), of course, has no exiting elements.

Think I understood this before and just forgot.

@gordonwoodhull
Copy link
Contributor

Thanks @kum-deepak, released in 3.0.5

gordonwoodhull added a commit that referenced this pull request Jul 9, 2018
@gordonwoodhull
Copy link
Contributor

(added a test too!)

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.

3 participants