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

Support for classes(), etc #139

Closed
tim-hutchinson opened this issue Oct 30, 2017 · 7 comments
Closed

Support for classes(), etc #139

tim-hutchinson opened this issue Oct 30, 2017 · 7 comments

Comments

@tim-hutchinson
Copy link
Contributor

This came up in #118. We've found it easier to work with tests that use the assertion library's comparison functions, rather than the comparison functions like hasClass, hasProp, etc.

I'd like to propose adding helpers that are more like html() and text() that simply return a consumable form of the element feature requested. IE, classes() would return an array of class names on the element. I've got an implementation of classes and can submit a PR for it and others that make sense, but wanted to see if it made sense before going further.

@eddyerburgh
Copy link
Member

I agree. Value assertions are more useful than boolean assertions.

We could replace hasClass, hasAttribute, and hasProp with classes, attributes, and props.

We've got a good opportunity to remove these before the official release.

The one method I think is useful is hasStyle, because it allows you to check hasStyle('color', 'white') instead of returning {color: 'rgb(255, 255, 255)}

What do people think?

@lmiller1990
Copy link
Member

lmiller1990 commented Oct 30, 2017

I haven't had much occasion to use attributes or props, but I'd be for including them - props seems like it might be handy. classes is solved in #118 , and should probably be renamed classes in light of the updated functionality.

I agree that hasStyle should stay, I have used it quite a lot (changing button colors, opacities, and the such).

@vouill
Copy link
Contributor

vouill commented Nov 6, 2017

I personally prefer the notation getClasses() but other than that i like it too.

@eddyerburgh
Copy link
Member

I also prefer the notation getClasses.

What do other people think?

@tim-hutchinson
Copy link
Contributor Author

The getX syntax seems different from the conventions used in this library already (html, text).

Alternatively, it could go fully the other way and implement the functions as getters, so they would expose a property-like API expect(wrapper.classes).to.eql(['foo', 'bar'])

@eddyerburgh
Copy link
Member

I've changed my mind, as you said we already have html and text methods.

@eddyerburgh
Copy link
Member

eddyerburgh commented Nov 21, 2017

This has been released in 1.0.0-beta.6 🎉. Thanks @tim-hutchinson 😀

Happy to continue discussing API, but I find:

wrapper.props().someProp

Is more elegant that

wrapper.getProps().someProp

Plus we can extend the props method by adding an optional param:

wrapper.props('someProp')

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

No branches or pull requests

4 participants