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
Handle clicks on SVG elements. Fix #8346. #8347
Conversation
Hi @dandv Thanks for Pull request. I noticed that the JavaScript tracking unit tests are failing, could you take a look and update tests accordingly, if the change is indeed correct? https://travis-ci.org/piwik/piwik/jobs/70837343 See readme about JS tests: https://github.com/piwik/piwik/tree/master/tests/javascript#javascript-tests |
Unfortunately I'm on a very tight deadline and can't dedicate much time to this. Hopefully the reproduction steps have been useful and someone more familiar with the code can come up with a better fix? |
I don't understand why that test fails, without looking at its code. Logically, all the patch does is pass-through the split className if className was a string, just as before. If it wasn't, it returns an empty array. What can className be in |
I think |
@tsteur: I think I got confused by |
It's very tricky to find JSLint errors unfortunately, I debugged and found this:
see eg http://stackoverflow.com/questions/6824895/jslint-error-unexpected-in-compare-with-undefined-or-use-the-hasownproperty and http://stackoverflow.com/questions/12179371/why-jslint-complains-about-this-code-and-how-should-i-fix-it Just FYI: After merging we need to minify the piwik.js |
We should also add a test for tracking clicks on SVG elements, but that's above my pay grade :) Happy my PR passed tests (I guess the screenshot differences are an expected failure). I'll let someone more familiar with test writing take over. |
Handle clicks on SVG elements. Fix #8346.
No description provided.