search

plumatic / dommy

A tiny ClojureScript DOM manipulation and event library
759 stars 74 forks source link

fix use of setAttribute/getAttribute for setting className on Element in... #6

Closed mpenet closed 11 years ago

mpenet commented 11 years ago

setAttribute to set className is unreliable on IE (depending on the version, there is also history of browser vendors juggling with the way get/setAttr work with class), the correct way to do this is to use the .className property, it is also possibly faster.

aria42 commented 11 years ago

This commit seems to have broken one of the tests

Error: Assert failed: (-> e (.getAttribute "class") (= "class1 class2"))

Happy to merge if you fix the test. The test itself may be bad, so feel free to change it.

mpenet commented 11 years ago

Odd:

mpenet@thinkbox:~/gitmu/dommy(set-attr-vs-classname)$ lein test Compiling ClojureScript.

lein test user

Testing user

Ran 0 tests containing 0 assertions. 0 failures, 0 errors. Running all ClojureScript tests. PASS simple-test

I am on linux by the way, maybe there is a difference in the test env then.

aria42 commented 11 years ago

In order to get tests to run, I have to run lein cljsbuild test. This will generate target/unit-test.js and then I run

phantomjs target/unit-test.js resources/test.html

The actual cljs unit test tests are in test/dommy/template_test.cljs

lein cljsbuild test is supposed to also run this phantomjs command, but doesn't seem to half the time

Happy to take any pull requests that clean the test situation up.

On Fri, Jan 18, 2013 at 9:07 AM, Max Penet notifications@github.com wrote:

Odd:

mpenet@thinkbox:~/gitmu/dommy(set-attr-vs-classname)$ lein test Compiling ClojureScript.

lein test user

Testing user

Ran 0 tests containing 0 assertions. 0 failures, 0 errors. Running all ClojureScript tests. PASS simple-test

I am on linux by the way, maybe there is a difference in the test env then.

— Reply to this email directly or view it on GitHubhttps://github.com/Prismatic/dommy/pull/6#issuecomment-12431376.

website: http://aria42.com

mpenet commented 11 years ago

ok got it, it wasnt obvious but empty className is an empty string, so the if-let fails. I'll update in a sec

mpenet commented 11 years ago

It should be ok this time. I also updated the tests to use className instead of getAttribute where it applies.

mpenet@thinkbox:~/gitmu/dommy(set-attr-vs-classname)$ lein cljsbuild test
Compiling ClojureScript.
Compiling "target/main.js" from "src"...
Successfully compiled "target/main.js" in 10.819739101 seconds.
Compiling "target/unit-test.js" from "test"...
Successfully compiled "target/unit-test.js" in 5.544849934 seconds.
Running all ClojureScript tests.
PASS simple-test

and separately just to be sure: 
mpenet@thinkbox:~/gitmu/dommy(set-attr-vs-classname)$ phantomjs target/unit-test.js resources/test.html
PASS simple-test