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

[fixed] Make the modal portal render into body again #176

Merged
merged 1 commit into from
May 17, 2016

Conversation

claydiffrient
Copy link
Contributor

@claydiffrient claydiffrient commented May 17, 2016

In c13fed9 behavior was changed
so that the modal portal ended up rendering into the app element.

In the default case where the appElement receives aria-hidden
when the modal is open, this meant that the modal itself was
also contained inside the aria-hidden element. This limits the
ability for screenreaders to read the modal contents.

Server-side rendering should not be affected by this change
because componentDidMount is only fired on the client-side thus
having a reference to document.body there should be acceptable.

Changes proposed:

  • Make the portal append to body rather than the app element

Upgrade Path (for changed or removed APIs):

  • Make sure that you are setting an app element (rather than using the default document.body) so that modal contents are not being aria-hidden

Acceptance Checklist:

  • All commits have been squashed to one.
  • 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.

In c13fed9 behavior was changed
so that the modal portal ended up rendering into the app element.

In the default case where the appElement receives aria-hidden
when the modal is open, this meant that the modal itself was
also contained inside the aria-hidden element.  This limits the
ability for screenreaders to read the modal contents.

Server-side rendering should not be affected by this change
because componentDidMount is only fired on the client-side thus
having a reference to document.body there should be acceptable.

Upgrade Path:
  - Make sure that you are setting an app element (rather than
    using the default document.body) so that modal contents are
    not being aria-hidden
@claydiffrient
Copy link
Contributor Author

@diasbruno Would you mind looking this over and giving your thoughts before I merge and cut a new version?

@diasbruno
Copy link
Collaborator

Sure!

@diasbruno
Copy link
Collaborator

It's looks good. Good to go!

@claydiffrient claydiffrient merged commit 9089a2d into master May 17, 2016
@claydiffrient claydiffrient deleted the append-to-body branch May 17, 2016 16:02
@ricardocosta
Copy link

Hello,

README.md should be updated with some urgency, because it's not clear how (or if possible) to have the modal rendered inside a specified DOM element.

Thanks!

rogeriopvl added a commit to rogeriopvl/react-modal that referenced this pull request Sep 7, 2016
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.

3 participants