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

[added] support Array, HTMLCollection and NodeList values for appElement #861

Merged
merged 2 commits into from Apr 2, 2021
Merged

Conversation

eemeli
Copy link
Contributor

@eemeli eemeli commented Mar 31, 2021

Changes proposed:

  • Hide multiple app elements, not just one
  • Accept Array, HTMLCollection & NodeList values for the appElement prop

While working with an application that uses more than one top-level element, it became obvious that providing a fully accessible experience via react-modal is challenging, as it's currently only able to hide one app element.

The fix for this is pretty simple, and should have no effect on existing users. The accepted shape of the appElement value now also includes Array, HTMLCollection (returned by document.getElementsBy*) and NodeList (returned by document.querySelectorAll).

As a result of this change, it becomes easier to use react-modal to provide a fully accessible experience.

Acceptance Checklist:

  • The commit message follows the guidelines in CONTRIBUTING.md.
  • Documentation (README.md) and examples have been updated as needed.
  • If this is a code change, a spec testing the functionality has been added.
  • If the commit message has [changed] or [removed], there is an upgrade path above.

@coveralls
Copy link

coveralls commented Mar 31, 2021

Coverage Status

Coverage increased (+0.2%) to 82.857% when pulling 3e950cd on eemeli:multi-root into c9d8e2d on reactjs:master.

package-lock.json Outdated Show resolved Hide resolved
@diasbruno
Copy link
Collaborator

Looks great. Bom trabalho!

Modal.setAppElement()...I'm worried this can potentially hold dead elements.

Any thoughts?

@eemeli
Copy link
Contributor Author

eemeli commented Apr 1, 2021

Modal.setAppElement()...I'm worried this can potentially hold dead elements.

That is a possibility. Proactively getting rid of dead elements would probably require adding something like a document.body.contains(...) check within the validateElement() call to act as a filter. It would be nice if a WeakSet could be used, but that's not iterable.

Should I add that sort of filter?

@diasbruno
Copy link
Collaborator

It would be nice if a WeakSet

Yeah, totally agree.

I don't know if we should be defensive in this case, or, just write it in the docs warning about it.

@diasbruno diasbruno merged commit d7083c5 into reactjs:master Apr 2, 2021
@diasbruno
Copy link
Collaborator

Thanks, @eemeli.

@eemeli
Copy link
Contributor Author

eemeli commented Apr 6, 2021

Any chance of getting a release including this sometime soon? 😇

@diasbruno
Copy link
Collaborator

Sorry for taking soooo long...released version 3.13.1.

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

Successfully merging this pull request may close these issues.

None yet

3 participants