Skip to content
This repository has been archived by the owner on Sep 7, 2020. It is now read-only.

Possible solution for #19. #25

Merged
merged 2 commits into from Nov 3, 2015
Merged

Possible solution for #19. #25

merged 2 commits into from Nov 3, 2015

Conversation

futuraprime
Copy link
Contributor

I'm not sure this is the best solution. It does appear to work, but this solution may have some unintended consequences as d3 updates the data in subsequent passes.

I'm not sure this is the best solution. It does appear to work,
but this solution may have some unintended consequences.
@Olical
Copy link
Owner

Olical commented Nov 3, 2015

Very interesting! It is indeed a bad idea to rely on private properties though, you're right. It could change easily and means react-faux-dom (which is currently just a generic DOM implementation) would contain D3 specific shims, which I'm pretty sure I've avoided up until now. Maybe instead of...

listener.call({ '__data__': data }, event)

We could use...

listener.call(this, event)

I'm pretty sure that would also fix it. Maybe this passing through of the this object was missing from the API which had a side effect of breaking this part of D3? This would then correlate the bug arising with my new listener code which probably broke the this proxy.

@futuraprime
Copy link
Contributor Author

Unfortunately, passing this doesn't work—it's undefined within the listener.

The first thing I tried was to pass syntheticEvent.target or the like—but unfortunately when React renders these elements it strips out the __data__ property, and I don't know a way of coaxing it into adding properties to DOM elements it creates (though I'm pretty new to React).

@Olical
Copy link
Owner

Olical commented Nov 3, 2015

But this does exist in the closure right? Where you're getting the original __data__ from? Is there a reason why we can't use that? Thanks a lot for taking the time to get something together though, I've just been way too busy the past couple of weeks to clear the current issues out.

@futuraprime
Copy link
Contributor Author

Oh! Yes, I see what you mean.

@Olical
Copy link
Owner

Olical commented Nov 3, 2015

Looks pretty sweet. Great job @futuraprime! I'm happy to merge if you are? I'll deploy right after.

@futuraprime
Copy link
Contributor Author

Great—thanks!

Olical added a commit that referenced this pull request Nov 3, 2015
@Olical Olical merged commit 512c27b into Olical:master Nov 3, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants