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

[changed] Replace appElement with getAppElement #360

Merged
merged 1 commit into from
Mar 26, 2017
Merged

Conversation

claydiffrient
Copy link
Contributor

@claydiffrient claydiffrient commented Mar 24, 2017

This makes getAppElement a required prop as well as makes
it a function that will be called expecting a DOMElement.

closes #287

This also takes some inspiration from #359 for handling arrays
of objects.

Fixes #[issue number].

Changes proposed:

  • Remove appElement in all it's forms
  • Add getAppElement as a function prop

Upgrade Path (for changed or removed APIs):

  • If you had specified an appElement via Modal.setAppElement,
    then you need to convert that to a getAppElement prop on the
    modal, this should be a function that returns either a single
    element or an array of elements.
  • If you had nothing specified you will need to add the getAppElement
    element to prevent breakages.

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.

});
} else {
appElement.removeAttribute('aria-hidden');
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

show() has the same effect if it sets ae.setAttribute('aria-hidden', 'false');, so hide() and show() could betoggle(appElement : DOM, value : bool).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call :)

This makes getAppElement a required prop as well as makes
it a function that will be called expecting a DOMElement.

closes #287

This also takes some inspiration from #359 for handling arrays
of objects.

Upgrade Path:
  - If you had specified an appElement via `Modal.setAppElement`,
    then you need to convert that to a getAppElement prop on the
    modal, this should be a function that returns either a single
    element or an array of elements.
  - If you had nothing specified you will need to add the getAppElement
    element to prevent beakages.
@dminuoso
Copy link

This solution elegantly tackles #133 too.

@claydiffrient claydiffrient merged commit baa1751 into master Mar 26, 2017
@claydiffrient claydiffrient deleted the getAppElement branch March 26, 2017 20:52
@dminuoso
Copy link

dminuoso commented Mar 27, 2017

One thing that I've been thinking about is the change to always require a getAppElement prop.

Does defaulting to () => document.body make sense perhaps?

@claydiffrient
Copy link
Contributor Author

@dminuoso No unfortunately it doesn't make sense. If you do that then all the body ends up hidden from screen readers which definitely makes it non-accessible. We've got that in v1 currently and there are lots of sites out there that get completely hidden from screen readers because of it.

@priyajeet
Copy link

priyajeet commented May 9, 2017

@claydiffrient are there any plans to back port this to version 1 for issue #133. Or when will V2 have a release soon (beta or otherwise)? Thanks!

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.

Remove Modal.setAppElement?
4 participants