@dandv opened this issue on July 14th 2015
@mattab commented on July 14th 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

@dandv commented on July 14th 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 commented on July 17th 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 commented on July 20th 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 commented on July 20th 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 commented on July 20th 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 commented on July 20th 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.

This issue was closed on July 20th 2015
Powered by GitHub Issue Mirror