Skip to content

querySelectorAll should return Elements #455

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
theqp opened this issue Aug 3, 2021 · 5 comments · Fixed by #560
Closed

querySelectorAll should return Elements #455

theqp opened this issue Aug 3, 2021 · 5 comments · Fixed by #560

Comments

@theqp
Copy link

theqp commented Aug 3, 2021

  1. It's inconsistent with querySelector which returns Element
  2. It's inconsistent with the MDN Web Docs

A non-live NodeList containing one Element object for each element that matches at least one of the specified selectors or an empty NodeList in case of no matches.

@raquo
Copy link
Contributor

raquo commented Aug 3, 2021

querySelectorAll returns a NodeList of Element-s:

def querySelectorAll(selectors: String): NodeList = js.native

This matches what the MDN page that you quoted says that the JavaScript method returns.

I'm not sure what you mean by "inconsistent with querySelector", but scala-js-dom doesn't have a choice here. This interface follows the design of the underlying Javascript methods. Javascript querySelectorAll method returns a NodeList, and that's what scala-js-dom has to return as well for that method definition.

@theqp
Copy link
Author

theqp commented Aug 3, 2021

@raquo The definition of NodeList is:
class NodeList extends DOMList[Node]
The MDN documentations states querySelector returns Elements, not Nodes thus I think it should return DOMList[Element]

@raquo
Copy link
Contributor

raquo commented Aug 3, 2021

Ah, I see what you mean now.

I guess one way to encode it would be class NodeList[N <: Node] extends DOMList[N].

@theqp
Copy link
Author

theqp commented Aug 3, 2021

@raquo
We already have an equivalent

trait NodeListOf[TNode <: Node] extends DOMList[TNode]

@armanbilge
Copy link
Member

@theqp or @raquo would you mind making a PR for this? Would be much appreciated! :)

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

Successfully merging a pull request may close this issue.

3 participants