Skip to content

Add a filter method #336

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
eddyerburgh opened this issue Jan 8, 2018 · 12 comments
Closed

Add a filter method #336

eddyerburgh opened this issue Jan 8, 2018 · 12 comments

Comments

@eddyerburgh
Copy link
Member

eddyerburgh commented Jan 8, 2018

There's a PR open to add a filter method to find: #334.

For example, users could filter elements that are visible:

const wrapperArr = wrapper.findAll('div')
const filteredArr = wrapperArr.filter(w => w.visible())

An alternative would be to make length a getter:

const wrapperArr = wrapper.findAll('div')
wrapperArr.wrappers = wrapperArr.wrappers.filter(w => w.visible())

My preference is to add a filter method. What are peoples thoughts?

@38elements
Copy link
Contributor

I think if the need and the use case of the feature are not explicitly indicated,
the feature should not be adopted and the feature only few people need should not be adopted.
If the filter function is added, its need and use case should be indicated.

@lmiller1990
Copy link
Member

I have never had any reason to filter on the array returned. Seems like it'll lead to a difficult to read test. If I want to know about a specific element, I use findAll.

I don't agree with adding a bunch of Array methods, as you said, the API gets larger and people might expect all Array methods. More API leads to more complexity. In that sense, making length a getter is better - but I still don't really see a major need for this feature. There may be use cases I haven't encountered, though.

@eddyerburgh
Copy link
Member Author

I think @Toilal has a good use case. He wants to filter all the elements that are visible. We're adding a visible method, so the use case would look like this:

const wrapperArr = wrapper.findAll('div')
const filteredArr = wrapperArr.filter(w => w.visible())

I can see that being useful

@Toilal
Copy link
Contributor

Toilal commented Jan 9, 2018

To better explain my use case, I'm currently writting a Tree component (http://vue-tree.pragmasphere.com/).

When clicking on tree node handle, it use v-show to hide children nodes when it's collapsed.

Problem is that it's actually very difficult to perform assertion on node visibility because it's not supported by CSS Selectors, and find()/findAll()/text() returns everything, even hidden elements. JQuery supports a custom CSS pseudo-class :visible to handle this use case, but I don't think you want to implement this.

I agree we should have less methods as possible in the Wrapper API, but having both visible() and filter() methods seems to be best option to test component that uses v-show.

Note that I can't use v-if in the component, as the nodes have to keep their state when hidden. At first, the component was using v-if and everything was smoothly testable, but after switching to v-show, testing this component is a pain.

@38elements
Copy link
Contributor

38elements commented Jan 9, 2018

My issue for the filter function seems to be different.
I think that there is the needs to filtering by using property Vue instanfce and the method of Wrapper not only CSS Selector.
It is possible to using Array.filter().
I considered the method which returns WrapperArray type is necessary or not.

@38elements
Copy link
Contributor

Currently, WrapperArray is a proxy of array of Wrapper.
The array of Wrapper is easier to understand and use for me.
I think it may be better that WrapperArray inherits Array.
For example, In the tree control, it may be necessary to confirm that the states of all trees are closed at the start.
Array.every() may be used.
How about WrapperArray inheriting Array ?

@Toilal
Copy link
Contributor

Toilal commented Jan 9, 2018

@38elements
Copy link
Contributor

It became possible at ES2015.

@38elements
Copy link
Contributor

38elements commented Jan 10, 2018

I think if moderate number of people think that they need the WrapperArray.filter(), it should be adopted.

@38elements
Copy link
Contributor

38elements commented Jan 10, 2018

@38elements
Copy link
Contributor

You can get WrapperArray class from WrapperArray instance.

const WrapperArray = wrapperArray.constructor

@phanan
Copy link
Member

phanan commented Jan 21, 2018

Can't say I'm a fan of adding (into core) these thin wrappers on top of what can already be done pretty easily via the contained .wrappers array. If we add filter, we should also add each and map etc. and the API will be quite bloated.

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

5 participants