[core] Create new @material-ui/private-theming package #25986
Conversation
|
@material-ui/core: parsed: +0.09% , gzip: +0.02% |
|
I'm not too keen on the idea of Regarding the different options available, I tried to poor more thoughts into it (and not reading this PR description to not be biased). Is Option A, viable?
It actually seems to be Option 1. |
How about we make this package private, and maybe rename it to |
|
@mnajdova This could work, the advantage would be that developers that depend on I would personally vote for no extra packages if possible. |
Correct. |
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
mnajdova
changed the title
|
@oliviertassinari @eps1lon are we onboard that we are good to go with Option 1 - making the use of If that is the case, we can go with reviewing with this PR, and as a follow up I would remove all dependencies of Edit: Should we name the new package to |
oliviertassinari
added
the
package: styles
👍 for it (BC on ThemeProvider and the import paths). This will require most of the existing projects in v4 to make the change, but the proposed alternative doesn't seem practical.
|
Agree on the name. The package will at least allow us to have common theme context and utils between the two packages without requiring developers to have both |
|
|
| @@ -0,0 +1,8 @@ | |||
| export { default as getThemeProps } from './getThemeProps'; | |||
There was a problem hiding this comment.
For the content of the package, did you consider keeping to its bare minimum, the React context singleton, and then, duplicate all the other modules? For instance, this one, it looks like we will want to duplicate it between the core and the legacy styles package.
There was a problem hiding this comment.
That makes sense, the only thing really required to be singleton is the React context. Let me update 👍
There was a problem hiding this comment.
Done, I've duplicated the utilities, the only things left in the new package, are the ThemeProvider, useTheme hook and the interface for the DefaultTheme.
mnajdova
changed the title
| expect(text()).to.equal('foobar'); | ||
| expect(themes).to.have.length(1); | ||
| }); | ||
|
|
||
| it('does not allow setting mui.nested manually', () => { |
There was a problem hiding this comment.
All tests except this one are moved to the @material-ui/private-theming package. This test had to stay here as it uses the makeStyles() utility which is not available there.
packages/material-ui-styles/src/getThemeProps/getThemeProps.spec.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
This PR will serve as a POC that we can remove the dependency of
@material-ui/stylesin@material-ui/core. Things planned to be done:ThemeProvider,ThemeContextanduseThemehook in a new@material-ui/themingpackage@material-ui/styles&@material-ui/corefor using the context from the new package@material-ui/styles&@material-ui/coreAfter this we have two options:
Option 1
Makes the use of
ThemeProviderrequired for the@material-ui/styles. In that case we will need to:@material-ui/core/styles:makeStyles,styledwithStyles&withThemein favor of the ones in@material-ui/stylesthat will use the theme from the contextOption 2Make everything work as previously by extracting more things in the@material-ui/themingpackageextractcreateMuiThemein the new package, so that we can createdefaultThemeoutside of thecore(are we fine with this?)safely remove the duplicated utilities in@material-ui/core/styles:makeStyles,styledwithStyles&withThemein favor of the ones in@material-ui/stylesthat will use thedefaultThemeI would go with Option 1 as a follow up PR.