-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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-infra] Support multiple tabs in demos #40901
Conversation
Netlify deploy previewhttps://deploy-preview-40901--material-ui.netlify.app/ Bundle size report |
41b0eab
to
0de3a88
Compare
This comment was marked as resolved.
This comment was marked as resolved.
98e3775
to
41b0eab
Compare
This is so good! Thanks for working on it! I wonder if we still need the collapse/expand button. Or perhaps it can simply adjust the max height of the code container instead of extracting the return value of the main function. cc @danilo-leal I've noticed that the JS/TS switch doesn't work. The JS source is always displayed. |
That is probably since the Combo Box demo has identical source for both cases; I've only added this to one demo for a proof of concept. I'll add it to a second with different sources to demonstrate JS/TS source switching working as expected |
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.
A quick review, great to see this making progress 👍
test/regressions/index.js
Outdated
'docs-components-autocomplete-data', // No components, contains only data for the demo | ||
'docs-components-transfer-list-utils', // Commont utilities for 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 think these should be handled automatically. It doesn't scale to ignore folders and non React components manually.
'docs-components-autocomplete-data', // No components, contains only data for the demo | |
'docs-components-transfer-list-utils', // Commont utilities for demos |
To ignore files that don't follow the ComponentName React convention.
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 think it should be a standalone PR, he hit the same pain with: docs-getting-started-templates-landing-page/getLPTheme.png
.
function not(a: readonly number[], b: readonly number[]) { | ||
return a.filter((value) => b.indexOf(value) === -1); | ||
} | ||
|
||
function intersection(a: readonly number[], b: readonly number[]) { | ||
return a.filter((value) => b.indexOf(value) !== -1); | ||
} | ||
|
||
function union(a: readonly number[], b: readonly number[]) { | ||
return [...a, ...not(b, a)]; | ||
} | ||
import { intersection, union, not } from './utils/helpers.js'; |
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 looks fine to test the PR out, but I don't think this should be deduplicate for the final version of the PR.
'no-console': ['off', { allow: ['info'] }], | ||
// not very friendly to prop forwarding |
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.
That file looks like technical debt to me. This should be in the root .eslintrc file
'no-console': ['off', { allow: ['info'] }], | |
// not very friendly to prop forwarding |
@@ -1,6 +1,7 @@ | |||
import * as React from 'react'; | |||
import TextField from '@mui/material/TextField'; | |||
import Autocomplete from '@mui/material/Autocomplete'; | |||
import top100Films from './data/top100Films.js'; |
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 would expect:
import top100Films from './data/top100Films.js'; | |
import top100Films from './data/top100Films.ts'; |
or likely:
import top100Films from './data/top100Films.js'; | |
import top100Films from './data/top100Films'; |
to keep it simple
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.
Do you mean that the relative imported package should always be a .ts
file?
What I'm thinking is if there is no extension, it would become a lot more complicated to figure out which file to load when resolving these relative imports in loader.js
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 assume you're importing .js
because it can be imported in both JS and TS.
IMO it can be kept as it is, and in further step, write imported files with TS and modify the docs:typescript:formatted
script to generate the imported file in JS, and update the import line
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.
@alexfauquette By as it is do you mean keep the .js
extension?
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.
yes
@@ -332,9 +332,25 @@ export default function DemoToolbar(props) { | |||
const handleSnackbarClose = () => { | |||
setSnackbarOpen(false); | |||
}; | |||
|
|||
const copyWithRelativeModules = React.useCallback( |
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.
Why have this in the scope of the component? Why not in the parent scope called from 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.
Do you mean why not move this function (including the clipboard-copy
dependency) to the parent scope of Demo.js
?
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.
To the root scope of DemoToolbar.js
packages/markdown/loader.js
Outdated
extractImports(demos[demoName].raw).forEach((importModuleID) => { | ||
// detect relative import | ||
// skip for modules that are not demos | ||
if (!toolbarHidden.has(moduleID.replace('./', ''))) { |
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.
So I guess this could ship as its own PR optimizing things?
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.
Do you mean taking this part apart into a separate PR focussed only on optimisation? I'd imagine this can stay, whereas future optimisation can go in a new PR
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 think we should have this as a standalone win/PR. If you go to https://mui.com/material-ui/react-grid/#interactive. Why are we loading the source to make that demo editable? Doesn't make sense.
I saw React Runner has a multi file demo at https://react-runner.vercel.app/#multi-files and tests https://github.com/nihgwu/react-runner/blob/974ebc932db7b7c7d59f1b50a79aed705efbf75a/packages/react-runner/src/__tests__/useRunner.test.tsx#L210, to see if there are things to learn from it. |
@bharatkashyap is there anything I can do to help this PR move forward? 😃 |
Haha, I guess not 😅 It needs some final work to address review feedback - I've planned to close it this week |
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.
👍
Pre-v1:
- Skipping demos without being editable [docs-infra] Skip resolving non-editable demos in build process #41585
- Skipping non React components in visual regressions (Material UI, Base UI, MUI X) [docs-infra] Skip non React components from visual regression tests #41584
- eslintrc root
For v1:
- TypeScript support
- Atomic commit for state change
For v2:
- Editable tabs
const codes = [tab1Code, tab2Code]; const scope = withFiles(baseScope, tab1Code, tab2Code).
withFiles
from https://github.com/nihgwu/react-runner/blob/974ebc932db7b7c7d59f1b50a79aed705efbf75a/website/src/components/MultiFilesExample/index.tsx#L27.
For a v3
- TS/JS switch using useTransition
@@ -0,0 +1,11 @@ | |||
export function not(a, b) { |
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.
export function not(a, b) { | |
export function not(a: readonly number[], b) { |
@@ -332,9 +332,25 @@ export default function DemoToolbar(props) { | |||
const handleSnackbarClose = () => { | |||
setSnackbarOpen(false); | |||
}; | |||
|
|||
const copyWithRelativeModules = React.useCallback( |
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.
To the root scope of DemoToolbar.js
packages/markdown/loader.js
Outdated
extractImports(demos[demoName].raw).forEach((importModuleID) => { | ||
// detect relative import | ||
// skip for modules that are not demos | ||
if (!toolbarHidden.has(moduleID.replace('./', ''))) { |
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 think we should have this as a standalone win/PR. If you go to https://mui.com/material-ui/react-grid/#interactive. Why are we loading the source to make that demo editable? Doesn't make sense.
test/regressions/index.js
Outdated
'docs-components-autocomplete-data', // No components, contains only data for the demo | ||
'docs-components-transfer-list-utils', // Commont utilities for 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 think it should be a standalone PR, he hit the same pain with: docs-getting-started-templates-landing-page/getLPTheme.png
.
@@ -1,6 +1,7 @@ | |||
import * as React from 'react'; | |||
import TextField from '@mui/material/TextField'; | |||
import Autocomplete from '@mui/material/Autocomplete'; | |||
import top100Films from './data/top100Films.js'; |
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 assume you're importing .js
because it can be imported in both JS and TS.
IMO it can be kept as it is, and in further step, write imported files with TS and modify the docs:typescript:formatted
script to generate the imported file in JS, and update the import line
Quick question: Once this PR is merged, can we bring the feature to MUI X directly? |
Yes, I tried the PR here: mui/mui-x#12714 |
packages/markdown/loader.js
Outdated
// If the demo is a JS demo, we can assume that the relative import is either | ||
// a `.js` or a `.jsx` file, with `.js` taking precedence over `.jsx` | ||
// likewise for TS demos, with `.ts` taking precedence over `.tsx` | ||
const extensions = variant === 'JS' ? ['.js', '.jsx'] : ['.ts', '.tsx', '.js', '.jsx']; |
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.
const extensions = variant === 'JS' ? ['.js', '.jsx'] : ['.ts', '.tsx', '.js', '.jsx']; | |
const extensions = variant === 'JS' ? ['.js', '.jsx', '.ts', '.tsx'] : ['.ts', '.tsx', '.js', '.jsx']; |
@danilo-leal This is okay to merge as a v1 for me, pending a review from you around how it looks 😅 |
Looks great to me! I only noticed that the borders on the code block/editor and the demo toolbar are a bit funky—they appear and disappear depending on the mode/tab selected. Check it out: Screen.Recording.2024-04-16.at.18.49.31.movThe demo toolbar has a top border in light mode. But, it seems the code block/editor loses its borders on the second tab. Screen.Recording.2024-04-16.at.18.49.59.movIn dark mode, the demo toolbar doesn't have both a top and bottom border anymore. I thought I had fixed these things on #41827 😅 I'll leave you to it as I'm having weird conflicts to pull the latest changes from this PR, so it should be faster if you can tackle it. Later design improvements can come iteratively right after, I think! |
@danilo-leal I tried to fix it in In light mode, the code has no border because the contrast between code and the background is enough In dark mode keep the border gray, |
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.
Let's 🚢 it!
Open to suggestions on how to make all files real-time editable, since the option I had in mind (which would require updating the
react-runner
instance scope Modules every time an import was changed) seems to me a bit too complex to become part of the v1 implementation.Preview (Autocomplete): https://deploy-preview-40901--material-ui.netlify.app/material-ui/react-autocomplete/