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

You can now add a class to an element that didn't already have a class. #63

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JulianBirch
Copy link

Hi, it's probably possible to make this code more efficent/elegant, but it's passing my manual tests.

Sorry, no test case because I'm having trouble getting your tests to run.

@cpetzold
Copy link
Contributor

cpetzold commented Jan 9, 2014

Hey Julian, can you explain the problem this is solving with a code sample?

@JulianBirch
Copy link
Author

(Not at a dev computer right now, so apologies if this is garbage)

<div id='hello'>World</div>
(dommy/add-class! (sel1 :#hello) :world)

should trigger it. The basic problem is that if the element doesn't have a class, then class-name returns null, which means you can't call .split on the result.

As I say, sadly your tests don't work on my machine... :(

@jeluard
Copy link

jeluard commented Jan 9, 2014

It might be specific to some browser? It's working fine for me without the patch.

@JulianBirch
Copy link
Author

It is, the code I've fixed is on the "doesn't support classlist" branch. The joke being that it affected Chrome when I wrote it, but Chrome now seems to support classlist. So, it only really affects IE versions now.

Anyway, as I said, the problem is that if you access className and there isn't a class, it returns nil rather than empty string.

@cpetzold
Copy link
Contributor

The split is on classes, not class-name. It should be fine if class-name returns nil. Can you paste an error?

Also, the tests should work now after switching over to clojurescript.test.

@JulianBirch
Copy link
Author

Sorry, it's class-index that assumes that class-name is not null. It calls .index on it.

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

Successfully merging this pull request may close these issues.

3 participants