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

Use react-overlays directly? #10479

Closed
jquense opened this issue Feb 28, 2018 · 4 comments
Closed

Use react-overlays directly? #10479

jquense opened this issue Feb 28, 2018 · 4 comments

Comments

@jquense
Copy link

jquense commented Feb 28, 2018

👋 hey there. So I noticed that ya'll are essentially inline forking react-overlays, I think it's great we have two big react ui frameworks using some of the same base code! Is there a reason though for the fork? I understand that needs vary, but it seems like both our teams would really benefit from using the same code base. For one we'd get some help upstream and not miss out on bug fixes you've found, and hopefully ya'll own a lot less code here.

Thoughts?

cc @taion

@jquense jquense changed the title Use react-overlays directly Use react-overlays directly? Feb 28, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 28, 2018

@jquense You are right, react-overlays has been an immense help with #9613. Thank you for all you work in the React world! I haven't done the effort of keeping track of the diffs. From what I can remember:

  • react-overlays is pretty solid.
  • Limiting the number of dependencies.
  • Reusing the publishing architecture.
  • Fixing the SSR support of the Portal.
  • Adding an extra .mui-fixed logic in the modal manager.
  • Changing the Modal API around the transition handling.
  • People started contributed changes on top of it, like [core] Remove direct references to window/document objects #10328.
  • Removing < IE11 support to save bundle size.
  • Removing React < 16.3 support for bundle size
  • Fix modal manager extension [ModalManager] uses instance (arrow) functions which makes hard to extend #10527
  • Removing the propTypes in production.
  • Having the control of the components so we control our destiny, like removing LegacyPortal or migrating to the new hooks of React 16.3.0.
  • We will soon work on making the scroll blocking and overlay optional.
  • Adding an option to disable the portal.
  • Allowing people to target the model style and properties with the theme.

@jquense
Copy link
Author

jquense commented Feb 28, 2018

I think a lot of those seem generic yeah? It'd be awesome to get some of those benefits upstream ;) :P

On a serious note though, I realize you need to keep you users and use-cases in mind (heck we are doing the same thing with react-bootstrap), and that can be tough unless you own code. I can't speak for everyone else (mainly @taion) but I'd be happy to make you a contributor/owner on react-overlays if that'd help consolidate some of the effort here (and give you confidence that changes could be made quickly). Having a few different stakeholders there might a bit more challenging than owning it all, but I think the benefits there would more than out weigh the costs!

Happy to chat through any specific concerns as well

@jquense
Copy link
Author

jquense commented Jul 9, 2018

Hey man, you are welcome to take the code and not contribute anything back but you need to at least meet the basic legal requirements for using the code. I don't see proper attribution on the code you've copy and pasted over here and iterated on.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 10, 2018

Hey Jason. I can add a Notice file if you would like too like Apache is doing.

I thought that linking react-overlay in the documentation: https://material-ui.com/utils/modals/

capture d ecran 2018-07-10 a 10 21 51

and in the code was enough: https://github.com/mui-org/material-ui/blob/a1c5eafcbdca7f347aeb5a2f27050d9f4b254de0/packages/material-ui/src/Modal/ModalManager.js#L67

you are welcome to take the code

Thank you for sharing your work!

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

No branches or pull requests

2 participants