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

Option to check a container too #14

Closed
vonagam opened this issue Sep 10, 2017 · 5 comments
Closed

Option to check a container too #14

vonagam opened this issue Sep 10, 2017 · 5 comments

Comments

@vonagam
Copy link
Contributor

vonagam commented Sep 10, 2017

Right now only nodes within a container get tested, but not a container itself. I have use case when i want a container to be possibly included in resulted array.

In current state i may get what i need, by retrieving tabbables of a parent of a container and then filtering results by inclusion of container. My concern is that of performance. Siblings of a container can be big, while container itself may be small or empty, it may result in much of unnecessary work.

Another way is to add option which will tell to prepend a container to candidates list if it matches one of selectors.

What are your opinion on adding such option? It is ok or is it out of scope of this library?

@vonagam
Copy link
Contributor Author

vonagam commented Sep 10, 2017

Just a note for other people, who may need to customize their querying (like in #10):

Found a third, hacky way. Basically right now tabbable takes one argument el. And from this supposed element it needs only two things: ownerDocument and querySelectorAll. So, you can just pass not an element, but an object with those keys, where querySelectorAll implements custom querying.

@davidtheclark
Copy link
Collaborator

@vonagam: I understand the use case. How about adding an option includeContainer: boolean? There aren't any options yet, but we could add a second argument that's an options object. I can't think of any good reason not to. I think that would solve your problem, right?

@vonagam
Copy link
Contributor Author

vonagam commented Sep 16, 2017

Yes. That would be perfect.

@davidtheclark
Copy link
Collaborator

👍 open to a PR to introduce this option.

@davidtheclark
Copy link
Collaborator

Closed by #15.

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

2 participants