[internal] Add createMuiStrictModeTheme#20523
Merged
Merged
Conversation
testing if unused imports are removed from the bundle
This reverts commit 0762f98.
|
@material-ui/core: parsed: +2.01% , gzip: +1.62% Details of bundle changes.Comparing: 29d9ace...f43ffe2 Details of page changes
|
oliviertassinari
left a comment
oliviertassinari
left a comment
Member
There was a problem hiding this comment.
Smart, +1 for using unstable until we shift work to v5.
|
|
||
| return ( | ||
| <Fade in={open} timeout={transitionDuration} {...other}> | ||
| <TransitionComponent in={open} timeout={transitionDuration} {...other}> |
Member
There was a problem hiding this comment.
Oops, I had forgotten about this one in #17047 (comment).
Member
Author
|
Bundle size looks good:
|
|
|
||
| return ( | ||
| <Transition | ||
| <TransitionComponent |
Member
There was a problem hiding this comment.
Should we call the root component prop Component to match the other non transition component convention?
Member
Author
There was a problem hiding this comment.
Not a fan of the generic term if the component has a specific task.
Member
There was a problem hiding this comment.
I wonder. It could feel surprising 🤔.
Member
Author
There was a problem hiding this comment.
That the component responsible for Transition is named TransitionComponent over Component? Is this seriously a concern?
Member
There was a problem hiding this comment.
Would it be like we change the convention for the style rule, naming the root class name .transition over .root?
Member
There was a problem hiding this comment.
What do you think of the idea to introduce a Components?: { Root: React.ComponentType, … } prop for v5 and empowering deep component customizations with styled component?
Similarly to what we do for the class names:
classNameprop at the rootclasses.rootobject at the rootclasses.xxxobject for the nested elements
we could have:
Componentprop at the rootComponents.Rootobject at the rootComponents.xxxobject for the nested components
NMinhNguyen
reviewed
Apr 13, 2020
oliviertassinari
added
internal
oliviertassinari
approved these changes
Apr 13, 2020
Contributor
|
@oliviertassinari @eps1lon |
This was referenced Apr 20, 2020
Member
Author
|
Generally these issues are indicative of a over-bundling. |
Contributor
|
Anyway it forces bundler to process two versions of the same package. |
Member
Author
What is the issue with that? A bundler should be able to do that. We have SemVer for that. Seems like you don't care about versioning packages. Maybe some bundlers can be configured to deduplicate every package to the highest version. |
Contributor
|
Nevermind. I missed you already published new version. |
This was referenced Apr 29, 2020
Contributor
|
Hey thank you all for this, much relief from my console warnings. May we get typescript types for this please? I noticed the Transitions all have types (ex. unstable_StrictModeTransition..) but not for createMuiStrictModeTheme. for now though will just be doing |
Member
Author
PRs welcome as usual. We're not really pursuing this right now since there are other issues that aren't fixable without a breaking change. |
oliviertassinari
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Possible solution for #13394 (comment)
I realized that a gist would be too hard to integrate if you already have a custom theme. We want to remove as much friction as possible since testing Material-UI in StrictMode is very useful.
So we're going with a (hopefully) tree-shakeable solution that won't affect prod bundles:
docs bundle should not only show marginal size increases.
We use the
<Component ComponentImplementation={FancyImplementation} />pattern for dependency injection. Bundle aliases would not be sufficient. All the added props are undocumented for the time being. Otherwise we would need to prefix them withunstable_and I'm too lazy for that 😉 So they stay private for now. Only the added components are public (withunstable_) since they are required if you build your own components on top of our transitions (like we do with e.g. the Notifications component).Related to #13394