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

Handle clicks on SVG elements. Fix #8346. #8347

Merged
merged 4 commits into from Jul 20, 2015
Merged

Conversation

dandv
Copy link
Contributor

@dandv dandv commented Jul 14, 2015

No description provided.

@mattab
Copy link
Member

mattab commented Jul 14, 2015

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

@mattab mattab added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Jul 14, 2015
@dandv
Copy link
Contributor Author

dandv commented Jul 14, 2015

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?

@dandv
Copy link
Contributor Author

dandv commented Jul 17, 2015

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 hasNodeCssClass? A JSDoc comment for that function would be great.

@tsteur
Copy link
Member

tsteur commented Jul 20, 2015

I think className in node is the problem. className can be any class. Eg when HTML is <div class="test test2"> and c className="test2" then it should return true. But "test2" in node won't be true

@dandv
Copy link
Contributor Author

dandv commented Jul 20, 2015

@tsteur: I think I got confused by className being both the name of the function argument that tells what class name to look for, and the Element method className. Any idea what causes the JSLint failure now?

@tsteur
Copy link
Member

tsteur commented Jul 20, 2015

It's very tricky to find JSLint errors unfortunately, I debugged and found this:

a: "in"b: undefinedc: undefinedcharacter: 54d: undefinedevidence: "                if (node && className && 'className' in node && node.className) {"id: "(error)"line: 1389raw: "Unexpected 'in'. Compare with undefined, or use the hasOwnProperty method instead."reason: "Unexpected 'in'. Compare with undefined, or use the hasOwnProperty method instead."__proto__: Object

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

@dandv
Copy link
Contributor Author

dandv commented Jul 20, 2015

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.

tsteur added a commit that referenced this pull request Jul 20, 2015
Handle clicks on SVG elements. Fix #8346.
@tsteur tsteur merged commit 69b1aae into matomo-org:master Jul 20, 2015
@dandv dandv deleted the patch-2 branch July 20, 2015 13:42
@mattab mattab added the answered For when a question was asked and we referred to forum or answered it. label Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answered For when a question was asked and we referred to forum or answered it. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants