-
-
Notifications
You must be signed in to change notification settings - Fork 32k
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
[docs] Allow codesandbox deploy for demos in X #23644
[docs] Allow codesandbox deploy for demos in X #23644
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.
Please inline it in the repo.
@eps1lon I can't inline it, we don't have customization points, as far as I know :/. I think that it would be ideal if each repository to have their custom logic. This approach seemed to be the least worse available. |
Then create it. But don't abstract the wrong thing. You're creating more maintenance burden for both repos instead of just one. |
@eps1lon Would it work with you if we use the context to inject customizations around how the export to code sandbox behaves? |
e7f1f50
to
805335d
Compare
@@ -62,19 +62,6 @@ const suggestions = [ | |||
}); | |||
}); | |||
|
|||
it('should support next dependencies', () => { | |||
expect(getDependencies(s1, { reactVersion: 'next' })).to.deep.equal({ |
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.
Doesn't seem relevant, support removed.
@@ -183,7 +180,7 @@ function getDependencies(raw, options = {}) { | |||
} | |||
} | |||
|
|||
includePeerDependencies(deps, versions); | |||
deps = includePeerDependencies(deps, versions); |
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.
Prefer immutability
if ( | ||
deps['@material-ui/lab'] || | ||
deps['@material-ui/x'] || | ||
deps['@material-ui/x-grid'] || | ||
deps['@material-ui/x-pickers'] || | ||
deps['@material-ui/x-tree-view'] || | ||
deps['@material-ui/data-grid'] | ||
) { | ||
deps['@material-ui/core'] = versions['@material-ui/core']; | ||
if (newDeps['@material-ui/lab']) { | ||
newDeps['@material-ui/core'] = versions['@material-ui/core']; | ||
} | ||
|
||
if (deps['@material-ui/x-grid-data-generator']) { | ||
deps['@material-ui/core'] = versions['@material-ui/core']; | ||
deps['@material-ui/icons'] = versions['@material-ui/icons']; | ||
deps['@material-ui/lab'] = versions['@material-ui/lab']; |
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.
Remove the logic, prefer hosting it in https://github.com/mui-org/material-ui-x.
feedback taken into account, add customization points so we can have a faster feedback loop
@@ -79,7 +79,7 @@ function getLanguageConfig(demoData) { | |||
} | |||
} | |||
|
|||
export default function getDemo(demoData) { | |||
export default function getDemoConfig(demoData) { |
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.
Per convention
805335d
to
249e61f
Compare
I don't think this is a good idea. Can we please revisit that once we recognize churn across the repositories? Until then we should just copy the code. |
What do you mean?
This would require to duplicate dozens of unrelated files (drill down in the dependency file chain). It would force duplicating the effort each time we release an improvement. We need to share more code, not less. |
Once we recognize that we change multiple locations at the same time we can look into how we abstract it. Here: If we change the duplication in both repositories often we can abstract the lines we often change.
Sounds great. Let's do it.
That's not generally true. |
Ok, so you want to identify the places where we need to customize the documentation for the use cases of another package. From what I understand it's what we have done so far, this pull request is another example. The duplication path is not an option because it reduces the constraints, it's critical that Material-UI X uses the infrastructure of the main repository. We don't have the bandwidth to duplicate the effort. So I think that we need to provide a customization API. The approach with the global seems simple enough. It allows moving code away from here (for packages that aren't hosted in this repository) |
You're harming willingly the repository for reasons you're not sharing with your fellow collaborators. |
I don't understand how it's harming the codebase. I believe it's the opposite, it enforces more constraints, helps with code sharing and consistency. There is one aspect that I can envision as remotely harmful, is at the scope of mui-org/material-ui in isolation if ignoring the overall benefit for mui-org that is positive. Changes to the "public API" here will have to take into account how the code is used in https://github.com/mui-org/material-ui-x. The creation of different repositories is part of this tradeoff. The motivation was to avoid potential confusion among developers around what's MIT and what's GPL-v3 or commercial. It comes with downsides, this one was anticipated. I think that we can still reevaluate if this was the right call in a couple of months, see if we should merge or not. |
These changes remove the custom code I have added for packages like |
The objective of this pull request is to get the same great DX of #22616 but for Material-UI X. Over the last few weeks, I have found myself going to the codesandbox-ci UI to get the published package many times. The workflow is especially useful when reviewing.
Once the changes are merged, I will cherry-pick it to
master
and complete the integration on mui/mui-x#613.