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

a problem with on("click",function) #19

Closed
wuxianliang opened this issue Oct 18, 2015 · 8 comments
Closed

a problem with on("click",function) #19

wuxianliang opened this issue Oct 18, 2015 · 8 comments
Assignees
Labels

Comments

@wuxianliang
Copy link

Dear @Olical
I am using react-faux-dom now to reproduce a bar chart.

chart.selectAll(".bar")
      .data(data)
      .enter()
        .append("rect")
        .attr("class", "bar")
        .attr("x", function(d) { return x(d.tradeDate.slice(5)); })
        .attr("width", x.rangeBand())
        .attr("y", function(d) { return y(d.turnoverVol); })
        .attr("height", function(d) { return height - y(d.turnoverVol); })
        .on("click", function(d, i) {console.log(d);console.log(i)});

when I click, "d" shows undefined, "i" shows correctly. I have tried console.log(d) in other attr, they all work well.
Am I wrong with something? How to use new event system? Thank you very much!

http://bl.ocks.org/mbostock/3885304

@Olical
Copy link
Owner

Olical commented Oct 18, 2015

Hm, no, that looks correct to me, I'll investigate. This is probably a bug, but it seems like something pretty big that would be hard to miss when I was writing my tests.

@Olical Olical self-assigned this Oct 18, 2015
@Olical Olical added the bug label Oct 18, 2015
@iuriikozuliak
Copy link

Same problem here, probably happens because of this line in d3 - https://github.com/mbostock/d3/blob/5b981a18db32938206b3579248c47205ecc94123/src/selection/on.js#L89
argumentz[0] contains correct value, but it's replaced with undefined.

Also we have wrong context in this function, using listener.apply(event.target, arguments) in mapper helps, but then this.__data__ is still missing, don't know yet how to fix it.

@Olical
Copy link
Owner

Olical commented Oct 21, 2015

I presume this.__data__ is supposed to be the selection? This is a weird section of code. Does this break all events then? I would imagine it's just because a context isn't being called correctly or arguments aren't being passed through. I will try to write a failing test for this, or if someone wants to submit one that'd be great too.

@wuxianliang
Copy link
Author

This problem is really blocking me. What is a failing test? Reproduce the problem? I'd like to help for fixing it.

@Olical
Copy link
Owner

Olical commented Nov 2, 2015

Hey, sorry about that. By failing test I mean adding a test to one of my test files asserting that and on click event with data should provide the correct arguments. That test would fail right now in theory since the arguments aren't being passed, then once we fix it we know the solution is correct, but don't worry I can do it.

I have been pretty busy recently and have been waiting for my new main development laptop to arrive. That should be at home when I finish work today, so I'll be able to catch up on open source over the next couple of days. I'm on holiday Thursday to Monday too so I may get some done during that time.

I apologise for the inconvenience, if I have time I'll fix it tonight, but it may have to be tomorrow. The problem will probably be around here just in case you want to take a look

return function (syntheticEvent) {
var event
if (syntheticEvent) {
event = syntheticEvent.nativeEvent
event.syntheticEvent = syntheticEvent
}
mapValues(listeners, function (listener) {
listener(event)
})
}
I think maybe it's not always passed a syntheticEvent argument. Maybe that's where d3 passes d and i?

@wuxianliang
Copy link
Author

Thank you very much! Your work will save many codes's time.

futuraprime added a commit to futuraprime/react-faux-dom that referenced this issue Nov 3, 2015
I'm not sure this is the best solution. It does appear to work,
but this solution may have some unintended consequences.
Olical added a commit that referenced this issue Nov 3, 2015
@Olical
Copy link
Owner

Olical commented Nov 3, 2015

@wuxianliang, can you confirm that @futuraprime fixed this with v2.1.0? It looks like it from the tests.

@wuxianliang
Copy link
Author

Thank you very much! @Olical @futuraprime , it works! I will upload a demo with react-faux-dom weeks later.

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

No branches or pull requests

3 participants