Skip to content

Support multiple classes in hasClass #118

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

Closed
lmiller1990 opened this issue Oct 23, 2017 · 14 comments
Closed

Support multiple classes in hasClass #118

lmiller1990 opened this issue Oct 23, 2017 · 14 comments

Comments

@lmiller1990
Copy link
Member

Hi,

I noticed that hasClass() only works for single word classes. For example, I did something like:

<template>
  <div :class="[ todo.isComplete ? 'complete todo' : incomplete todo']">
    Text
  </div>
</template>

And because hasClass() does

return !!(this.element && this.element.classList.contains(className))

You cannot find multiword classes. This was a little inconvenient since I mainly use semantic-ui, which uses longer multiword names like ui eight wide compact grid. Being able to assert the full class name makes my tests more understandable.

My proposal is to all hasClass to work with multiword classes. This seems like a pretty easy change and I'd like to tackle it if the maintainer(s) are okay with it.

@callumacrae
Copy link
Contributor

Multi-word classes are… not a thing. It's just multiple class names! With semantic-ui, are class="complete todo" and class="todo complete" the same thing?

If they're not the same thing, try blabla.is('[class*="complete todo"]');

@lmiller1990
Copy link
Member Author

lmiller1990 commented Oct 23, 2017

@callumacrae Thanks for the reply. In many cases the order does have an impact in semantic-ui, but I think those are mostly weird edge cases, probably not a good use case to argue for this feature. For now I changed the title of the issue to be more understandable.

I don't understand what your second comment means - I haven't seen that syntax, how can I use it to assert my wrapper has the class "complete todo" with the existing tools in vue-test-utils? Maybe I could pull another library in to do the comparison but other than doing two hasClass() calls, I don't see how I can achieve what I'm trying to.

If it's outside of the scope of vue-test-utils that's okay too, but I feel like I should be able to do wrapper.hasClass('multiple classes') and return true.

@lmiller1990 lmiller1990 changed the title Support classes with multiple words in hasClass Support multiple classes in hasClass Oct 23, 2017
@callumacrae
Copy link
Contributor

[class*="complete todo"] is a CSS selector that would match any element where the class attribute contains complete todo (but not todo complete): https://developer.mozilla.org/en-US/docs/Web/CSS/Attribute_selectors

I agree that it would be useful for hasClass to match multiple classes - I was just pointing out that if you want to check for order, there's already a way to do that. A selector like the above is also probably how semantic-ui deals with it as well.

@lmiller1990
Copy link
Member Author

Right, thanks for the clarification. I was more concerned about checking for the full two classes being present, not so much the order - in this case 'incomplete todo' vs 'complete todo'. I know I can just workaround by checking for incomplete or complete in this case, but I still think it'd be a nice addition.

@eddyerburgh
Copy link
Member

Yep we could do this @lmiller1990. Are you able to make a PR?

@lmiller1990
Copy link
Member Author

Sure, will look into it this week!

@lmiller1990
Copy link
Member Author

lmiller1990 commented Oct 24, 2017

Here is my first attempt @eddyerburgh

dev...lmiller1990:dev

Accidentally changed stuff in package.json, will revert. Added a test and all others are passing - if you can take a look and let me know if you are satisfied with the way I implemented the logic, that'd be great.

Will make the PR after I've gotten your feedback had a chance to run all the tests (for some reason having problems on my windows machine running the typescript types tests).

Edit: can probably make more readable by using:

.every((val, i, arr) => val === arr[0])
instead of
.reduce((a, b) => a === b ? a : NaN)

What I am doing is seeing if the entire array contains the same values - all true or all false`. Both methods were taken from here.

@eddyerburgh
Copy link
Member

@lmiller1990 If you make a PR I can review it in GitHub. But it looks good so far. You can use every instead of map and reduce:

targetClass.split(' ').every(target => element.classList.contains(target))

We can discuss further when you make a PR

@lmiller1990
Copy link
Member Author

Done #123 @eddyerburgh

@eddyerburgh
Copy link
Member

At the moment this will return true:

<template>
  <div class="class-a class-b" />
</template>
const wrapper = mount(Component)
wrapper.hasClass('class-b class-a')

I don't think this is the behavior we want. Do others agree?

@tim-hutchinson
Copy link
Contributor

tim-hutchinson commented Oct 25, 2017

We're handling this by using a classes function that returns the list of class strings as an array. This allows us to take advantage of the assertion libraries being used for handling the specific assertions, rather than replicating that functionality in the utility library.

Ex:

// <div class="class-a class-b class-c">
expect(wrapper.classes()).to.include.members(['class-b', 'class-a']); // True
expect(wrapper.classes()).to.include.ordered.members(['class-b', 'class-a']); // False

The other advantage this gives is clearer output on failure. With a failure on hasClass, our output is currently expected false to equal true. When we use the assertion library's array functions, output looks like expected [ 'class-a' 'class-b' 'class-c' ] to include 'foobar' which is much clearer to debug.

@callumacrae
Copy link
Contributor

@eddyerburgh I disagree, I would expect the current behaviour to be the desired behaviour. It's testing that the element has those classes, not that the class attribute contains that string.

Perhaps it would be clearer if the syntax were more like wrapper.hasClass(['class-b', 'class-a']).

When order matters I'd expect something more like wrapper.hasAttribute('class', 'class-b class-a').

@lmiller1990
Copy link
Member Author

Intuitively, I don't think the order should matter - in any css lib, like boostrap, I'd usually do btn btn-pimary - however since the other way around is perfectly valid and achieves the desired result, I think that hasClass should mirror that.

Although I also think @tim-hutchinson raises a valid point. I'm interested in seeing what other people think.

@eddyerburgh
Copy link
Member

Ok, I agree. Closing the issue as it's been merged 🙂

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

No branches or pull requests

4 participants