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] ability to change default 'ReactModalPortal' class #208

Merged
merged 1 commit into from
Aug 10, 2016

Conversation

manuhabitela
Copy link
Contributor

@manuhabitela manuhabitela commented Aug 8, 2016

Changes proposed:

  • new portalClassName prop to change the ReactModalPortal class.

Upgrade Path (for changed or removed APIs):

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.

Thanks for this project!

What do you think about adding this?

I think this is useful because:

  • code can be cleaner when also using custom content and overlay classnames. If using BEM classes, it makes more sense to have a DOM like .CustomModal > (.CustomModal-content + .CustomModal-overlay) and not like .ReactModalPortal > (.CustomModal-content + .CustomModal-overlay).
  • my current use case is opening a modal from a content-script in an browser extension. I want to be able to target the portal DOM element precisely in order to prevent conflicts with the page css.

Few questions on my side:

  • I went with "portalClassName" but I'm not sure if this is really speaking to everyone.
  • I added a brief sentence about the change in the README but didn't add anything in the examples. I figured the readme change would be enough, but let me know if you don't think so.

A new portalClassName prop in the <Modal> component lets us change
the wrapper className.
@claydiffrient
Copy link
Contributor

Looks good to me and I see no reason this can't go through.

@claydiffrient claydiffrient merged commit e5b0181 into reactjs:master Aug 10, 2016
felixgirault pushed a commit to DISIC/assistant-rgaa that referenced this pull request Jun 26, 2017
using a fork of react-modal for now, waiting for them to make a new
build with our merged PR reactjs/react-modal#208

ref #55245
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.

2 participants