-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[NoSsr] Move NoSsr to the Unstyled package #27356
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could spot only one issue, apart from that looks good to me 👍
@@ -2,6 +2,7 @@ import * as React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import { elementTypeAcceptingRef } from '@material-ui/utils'; | |||
import { useThemeProps } from '@material-ui/system'; | |||
import { NoSsr } from '@material-ui/unstyled'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since @material-ui/unstyled
is meant to have a nonnegligible package size. I think that we can import deep to force three-shaking in dev mode.
import { NoSsr } from '@material-ui/unstyled'; | |
import NoSsr from '@material-ui/unstyled/NoSsr'; |
@@ -1,5 +1,5 @@ | |||
import * as React from 'react'; | |||
import NoSsr from '@material-ui/core/NoSsr'; | |||
import NoSsr from '@material-ui/unstyled/NoSsr'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not consistent with what we do for the portal that is in the same configuration: https://next.material-ui.com/components/portal/. And inconsistencies open doors for discussion: why? what's best?
Basically, with Portal, we document the imports from core. We also add a dedicated unstyled section: https://next.material-ui.com/components/portal/#unstyled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My rationale was that the components lives in the Unstyled package and is reexported just for compatibility. Having a clear indication that this component can be imported from Unstyled will make the developers use a minimal set of dependencies (i.e. the whole Core is not needed when using code from this demo).
There is a tradeoff, though - some may find it confusing that some components are imported from Core and some from Unstyled. I can make it more clear you can use either option.
What were the arguments for using @material-ui/core
in Portal's demos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e. the whole Core is not needed when using code from this demo
What do you mean by "whole"?
What were the arguments for using @material-ui/core in Portal's demos?
The idea was to allow developers to find everything under @material-ui/core
so they don't have to think about where to import what. For instance, they don't have to install @material-ui/unstyled
if they only use MD.
Definitely a tradeoff, not clear what's best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh actually, how about:
- We document from core here for convenience, no extra package to install, to figure out where to import.
- Later, we create an isolated docs section for unstyled, different documentation, for a different audience, with different imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "whole"?
Perhaps not the best wording. I meant that it's not necessary to even have Core as a dependency (assuming we also use Box
from System).
Oh actually, how about:
We document from core here for convenience, no extra package to install, to figure out where to import.
Later, we create an isolated docs section for unstyled, different documentation, for a different audience, with different imports
Sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @danilo-leal
Moved the NoSsr component to the unstyled package and updated all references.
One chunk of mui/base-ui#10.
Preview: https://deploy-preview-27356--material-ui.netlify.app/components/no-ssr/#unstyled