[docs] Support live demo editing

[nihgwu]

• [x] I have followed (at least) the PR section of the contributing guide.

Preview: https://deploy-preview-32107--material-ui.netlify.app/components/buttons/

a POC to add live editing for components docs, using react-runner

Code highlighting keeps the same, and with that we don't need a loader to load the demos anymore, just run the code

Fix #26476.

[mui-bot]

No bundle size changes

Generated by :no_entry_sign: dangerJS against 66da51bbc9f5ff520ad18d325be2e392aedbfc9a

[michaldudak]

There is an ongoing effort to make the demos editable: #30873 and #29885.

[nihgwu]

@michaldudak Sorry I didn't know that, but you can see my change is much smaller and simpler than that PR, and that PR doesn't support SSR

[michaldudak]

@bharatkashyap could you take a look at this and coordinate your efforts?

[nihgwu]

@michaldudak can you reopen it and I'll fix the failing builds, and then you can compare, at least it works well locally for Button/Autocomplete, builds fail probably because I missed some imports

[mbrookes]

Thanks for working on it. I was able to try it out locally. A few minor issues (there may well be others):

• The preview isn't editable.
• The JS code is replaced with TS code.
• The editor has a keyboard focus indicator when selected by mouse.
• A bunch of symbols have an wiggly underline as if they're incorrect.
• It has a memory leak:

error

<--- Last few GCs ---> [95270:0x1049e0000] 86058467 ms: Mark-sweep (reduce) 1984.0 (2036.8) -> 1977.3 (2030.3) MB, 1116.6 / 0.0 ms (average mu = 0.269, current mu = 0.139) allocation failure GC in old space requested [95270:0x1049e0000] 86059705 ms: Mark-sweep (reduce) 1977.3 (2029.3) -> 1977.2 (2030.0) MB, 1237.9 / 0.0 ms (average mu = 0.164, current mu = 0.000) allocation failure GC in old space requested <--- JS stacktrace ---> FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory 1: 0x10130c5e5 node::Abort() (.cold.1) [/usr/local/bin/node] 2: 0x1000b2289 node::Abort() [/usr/local/bin/node] 3: 0x1000b23ef node::OnFatalError(char const*, char const*) [/usr/local/bin/node] 4: 0x1001f68c7 v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [/usr/local/bin/node] 5: 0x1001f6863 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [/usr/local/bin/node] 6: 0x1003a47e5 v8::internal::Heap::FatalProcessOutOfMemory(char const*) [/usr/local/bin/node] 7: 0x1003a628a v8::internal::Heap::RecomputeLimits(v8::internal::GarbageCollector) [/usr/local/bin/node] 8: 0x1003a19b5 v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::GCCallbackFlags) [/usr/local/bin/node] 9: 0x10039f2e0 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [/usr/local/bin/node] 10: 0x10039fc92 v8::internal::Heap::CollectAllAvailableGarbage(v8::internal::GarbageCollectionReason) [/usr/local/bin/node] 11: 0x1003adaae v8::internal::Heap::AllocateRawWithRetryOrFailSlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [/usr/local/bin/node] 12: 0x100374fc7 v8::internal::FactoryBase::NewFixedArrayWithFiller(v8::internal::Handle, int, v8::internal::Handle, v8::internal::AllocationType) [/usr/local/bin/node] 13: 0x1005f5a50 v8::internal::OrderedHashTable::Rehash(v8::internal::Isolate*, v8::internal::Handle, int) [/usr/local/bin/node] 14: 0x1006e18cf v8::internal::Runtime_MapGrow(int, unsigned long*, v8::internal::Isolate*) [/usr/local/bin/node] 15: 0x100a81a79 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_NoBuiltinExit [/usr/local/bin/node] error Command failed with signal "SIGABRT".

I presume these are all fixable. Other than that it appears to work as expected, so the main considerations vs the alternative is bundle size. react-runner is using sucrase, so that's a good sign.

[nihgwu]

@mbrookes Thanks for trying, I have no idea why the builds failed, I can build & export successfully locally, regarding the issues, right now it's just a POC, and ofc we can fix them, but for the first one I'd prefer keep JSX preview uneditable, live editing is just a complement not necessary, so it's fine to provide the very basic functionalities as before, if you want try by themselves, then expand it to full mode

As I mentioned in the description, probably we can reduce the build time a lot by removing the demos builds and use react-runner's JIT to run them, right now it's super slow, I didn't dive in deep, but we can make the markdown loader super simpler without black magic

[mbrookes]

but for the first one I'd prefer keep JSX preview uneditable

This is on the "must have" list, but ofc no need to add it until this approach is proven.

@bharatkashyap Could you have a look?

[Janpot]

I've reran the netlify build with success this time: https://deploy-preview-32107--material-ui.netlify.app/ Of the top of my mind, some of things this PR is missing is that can explain the reduced complexity compared to the existing PRs:

1. Preview editing is missing
2. It's not loading the react-runner asynchronously
3. It supports only four dependencies react, @mui/material, @mui/icons-material, @mui/lab. How do you plan to support all the dependencies all of the demo's are using while simultaneously avoiding to load them on every page?

[bharatkashyap]

@nihgwu This is a promising approach, based on the bundle size of react-runner versus jarle.

As @mbrookes mentioned, making the JSX preview editable is pretty much a must have (which is why #30873 loads the editor and runner on first load, whereas the earlier #29885 loads these components after user input and swaps out the original, static preview).

Also, #30873 has a way to not statically import all the dependencies at once like this one does at present (as @Janpot mentioned). I'm sure we can take this ahead if you take a shot at addressing these two issues

[nihgwu]

@Janpot @bharatkashyap aysnc loading is never a problem for react-runner 😉 , checkout this demo, probably I should work on #30873 but revert all the changes on the variable orders if you do need live editing of JSX preview, though I still don't like the idea, as it's not the real code it's running against

[nihgwu]

OK, the problem with async module resolution is that you are not allowed to import other components other than the components included in the initial code

And it doesn't support SSR Initial loading of all packages would be a problem for MUI as it's really huge

The problem with live editing of JSX preview, apart from the above one, the implementation in #30873 is not good as it's simply replacing the JSX part from the raw code, which means the JSX code and row code are highly coupled, and you can't make any mistake when editing them, not even a whitespace, if you do need that, of cause I can inject those components into scope instead of imports, but I think we can postpone it for following PRs, @oliviertassinari WDYT?

[nihgwu]

Regarding the bundle size, the biggest one is @mui/icons-materials 662.2KB, then @mui/materials 128KB, and then @mui/lab 39KB, I think it's fine to load the later two directly, but lazy loading icons, and use placeholder in initial rendering (we can still import icons already in raw code), then I think it should be fine for the bundle size impact given the fact we are already using part of them in other pages for layout, and we are using Service Worker to cache the resources, I think it's a better UX then only support a limited components which already imported in raw code, the problem is that if user is exploring the demo of Button with Icon, and want to try other icons, but it's simply impossible with #30873 's approach

Of cause I can use the same strategy as #30873 to import on-demand by the loader

UPDATE: Never mind, I've switched to on-demand imports 🎉

[oliviertassinari]

@nihgwu Happy to see you push our previous explorations on the problem further. We had a look at it with @bharatkashyap, we are happy to continue with this PR to close #26476.

Here are what seems to be missing before we can merge this PR:

• [ ] The default focus outline aesthetic doesn't look great, I think that we could personalize it to match the docs design

• [ ] The undo (CTRL+Z) and redo shortcuts don't work. It's frustrating.
• [ ] The caret is not visible when the code is empty

• [ ] The error handling logic could be improved. I think that it's frustrating to have its caret moves as soon as an error is done. I would advocate for keeping the code above the caret stable, doing it like https://sancho-ui.com/components/button/.

---

I have just rebased on HEAD.

---

OK, the problem with async module resolution is that you are not allowed to import other components other than the components included in the initial code

Only allowing to import existing components used in the demo sounds great. This feature is meant for a quick editing, it's not CodeSandbox.

[nihgwu]

@oliviertassinari yes I'm aware of the undo issue(it's a bug in use-editable), intended to fix when this PR is approved to continue(probably switch back to react-simple-code-editor in my early commit) , regarding error handling, what about showing it in the button of demo?, the problem with in sancho-ui is that it's highly possible to be hidden from the viewport

Also I have a stashed change in local to remove the compile the demoComponents as now the demo is fully served by react-runner, I think it would decrease the build time a bit

[oliviertassinari]

the problem with in sancho-ui is that it's highly possible to be hidden from the viewport

For the code preview is limited to 10 lines, so it won't be an issue, and for the code once expanded, then we could think of a small indicator to signal that there is a bug (no layout shift) and maybe to reduce the height of the code so the error shown at the bottom is more likely to be visible.

[nihgwu]

I mean like the following, then there won't be any layout shift, and editor's focus won't be distracted to 3 places

[oliviertassinari]

What I mean by layout shift is the ability to type code, without being distracted. When you write a new line, until you finish it, it won't be valid, but this error should not distract the developer, IMHO.

[nihgwu]

@oliviertassinari I've moved the error to bottom BTW I've moved demo compilation, we can remove demoComponents from page source code, I want to change it in a follow up to keep this PR small, or should I just change it in this PR? I can rename it to demoScope and use it instead of demos.scope Or we add a live: boolean to Demo then we can decide if it's served by react-runner or compiled by babel, anyway this could be done as a followup

[bharatkashyap]

@nihgwu There's a lag when editing large demos, for example. For me, there's a delay between editing the code and the change appearing in the preview.

This also shows up in the #30873, so it might not be a react-runner issue

[nihgwu]

@bharatkashyap looks OK on my laptop, probably we can move the data to another file to reduce the highlight time, and focus on component itself

[nihgwu]

@bharatkashyap @oliviertassinari So what's the plan 😉

[bharatkashyap]

@nihgwu The approach looks good to me (react-runner with async imports and editable JSX previews borrowed from #30873). There will be a significant bundle size jump with editable JSX previews, @michaldudak could you chime in on this aspect: Is it an acceptable trade-off?

Did we reach a settlement around the positioning of the error? Should be a good idea to create a list of pending issues in a commone on the PR and merge it post closure.

[nihgwu]

@bharatkashyap why do you think there will be a significant bundle size increase? No we only import the used modules as before, so the only addition is react-runner + react-simple-code-editor, which is about 43kb, and that's unavoidable, unless you won't to lazy load them, then it won't support SSR

Regarding the error position, it's very similar to what we have for form inputs in Mui, at the bottom of input control, and if we have other concerns we can always address them as a followup

[bharatkashyap]

@nihgwu Looks good to me (incl. bundle size shift) and my personal preference for the error position notwithstanding.

@siriwatknp @mnajdova This PR has merged the implementation approach from #30873 and replaced jarle with react-runner. I think we can consider it as the best candidate for merging and mark the other two (#30873, #29885) as closed. Looking for your opinions on this.

[mnajdova]

Overall looks good. Few things that probably could be improved.

1. Should we by default add all imports valid from the @mui packages in the scope? Will this affect perf? I feel it is limiting not being able to import a component that has not been in the initial code.
2. The error message is a bit distracting, appearing on each key stroke. Could we debounce it?
3. I feel like the error message design could be improved (for example using red background with white text) cc @danilo-leal can give better feedback on this by me :D

Great job @nihgwu

[mnajdova]

Few more thought I had after trying it a bit more.

1. When you click to edit the preview code, should we automatically open and show the whole code for the demo?
2. As this is a new feature, should we maybe show a small message somewhere around the demo code when we hover over it to indicate that it is editable? The mouse changes to cursor, but will this be enough of a indicator?

[nihgwu]

@mnajdova

Should we by default add all imports valid from the https://github.com/mui packages in the scope?

https://github.com/mui/material-ui/pull/32107#issuecomment-1104444006

The error message is a bit distracting, appearing on each key stroke. Could we debounce it?

Ofc we can debounce, not sure if it's really need though

[mbrookes]

@mnajdova

When you click to edit the preview code, should we automatically open and show the whole code for the demo?

https://github.com/mui/material-ui/pull/24640#issuecomment-770206949

Regarding expanding the code editor when clicking the inline preview, I doubt it's an option we can release. The inline preview allows easy copy and paste. It's no longer possible. The scroll position and cursor position state is lost at mouse down.

I think the cursor and scroll position could be fixed (from what I recall when looking at it last year), but not the copy issue, so I switched to the editable preview instead.

On a related note, should the "Copy the source" button copy the edited source rather than the original? Not sure if anyone actually uses that button – personally my instinct would be to highlight the needed code and CmdC (Ctrl-C). We might have stats in Google Analytics if that action is tagged, but I haven't checked.

I noticed also that edits in the preview are lost when opening the full source, but it might be nice to maintain them. (To be fair, my attempt didn't.)

Do we need a "reset source" button if the code has been edited, or is page reload good enough?

[mbrookes]

@nihgwu

Ofc we can debounce, not sure if it's really need though

I think it would be nice to have, but the reality is that most if the time, hesitating to think will cause react-runner to throw up the error. I like that react runner doesn't replace the working demo with an error the way that Jarle does though.

It would be good to try debouncing to see how it feels compared to without. You could use https://github.com/mui/material-ui/blob/master/packages/mui-material/src/utils/debounce.js

[nihgwu]

@mbrookes I don't add debounce to achieve insanely instant feedback on editing, and the last working demo will still available even there is error, but if you like that, I'll add it

On a related note, should the "Copy the source" button copy the edited source rather than the original?

TBH this button is confusing to me, I'd expect I'll copy the JSX preview on preview mode, but it doesn't, and if we changed the code, what would we expect when click editing in CSB or StackBlitz? So I just left it as it was, if you have other idea, you can pick it up in a followup

I noticed also that edits in the preview are lost when opening the full source, but it might be nice to maintain them. (To be fair, my attempt didn't.)

Personally I don't like the idea, for me they are completely two different things, why should we maintain the editing state, and how? and should we maintain the state on collapsing?

Do we need a "reset source" button if the code has been edited, or is page reload good enough?

There is a Reset Demo button at the end of toolbar, and it does the job

[mbrookes]

I don't add debounce to achieve insanely instant feedback on editing

I guess it depends on whether it's possible to distinguish between immediate rendering of valid code, and debouncing feedback on error.

I'd expect I'll copy the JSX preview on preview mode

I wondered about that as well. Personally I'd be keen to get rid of the button if we have analytics data that supports that it isn't heavily used vs select/copy.

why should we maintain the editing state, and how?

Why? Because to me it breaks expectations not to. If I edit some code in a code editor, and zoom, scroll or otherwise change my view, I wouldn't expect to lose my changes, so neither should we here. I don't understand the "how?" question though.

and should we maintain the state on collapsing?

Why not? We already extract the preview, so it's just a case of applying that algo to the edited demo. However I doubt that will be a common concern, so we could wait for someone to raise it.

[nihgwu]

I guess it depends on whether it's possible to distinguish between immediate rendering of valid code, and debouncing feedback on error.

Good suggestion, I'll make the change

Why? Because to me it breaks expectations not to. If I edit some code in a code editor, and zoom, scroll or otherwise change my view, I wouldn't expect to lose my changes, so neither should we here. I don't understand the "how?" question though.

For me if we do that then it breaks my expectation ;p. For me I think we are editing two different files. I asked how as IMO it's super tricky to do that, for example, we can use any valid form to write the demo in preview mode, like coping the full code to preview mode, then what should we do on expanding? How to merge the state? And if you have a closer look at current implementation to support JSX preview live editing, it's not a simple replacement

[mbrookes]

we can use any valid form to write the demo in preview mode, like cop[y]ing the full code to preview mode

The code preview is still dependent on the surrounding code, so that would just result in an error.

How to merge the state?

The same way it's done for the code preview to be rendered – simply open that modified code in the editor. Or am I missing something obvious?

[nihgwu]

@mbrookes no it's not, e.g. I change the code in preview mode to export default () => <Button />, it will show a button, but not inline JSX anymore, how to surround it? I'm wrong on this

Should we merge the change on changing language then? I don't think that's viable

The same way it's done for the code preview to be rendered

not that simple, the JSX preview code is extracted manually in source code format, like ButtonDemo.tsx.preview, and in order to support JSX live editing, we removed the leading space and replace the matching part in full code, I don't think there is a easy way to apply changed JSX code to full code while keeping correct indent, ofc we can make it, but I doubt if it's worth to do that

[mbrookes]

Should we merge the change on changing language then?

If you mean between TS and JS, then no, I don't think that's practical.

I don't think there is a easy way to apply changed JSX code to full code while keeping correct indent

Prettier could fix the indent. This may have been done already in a previous iteration, but I'm not sure about it.

[nihgwu]

@mbrookes Are you sure to include a package with 123KB just for that? I've checked it's not implemented in. previous iterations, also even it's fine to save changes on expanding, but it's not on collapsing, as in full mode we are free to change any place not only the JSX preview part, that means we need to keep 3 copies, JSX preview code, surrounding code in JS and surrounding code in TS, and assemble them accordingly, in my mind nothing is impossible, but as I said I doubt if it's worth to do that, it's way to make it too complicated

[mbrookes]

For later then

[mnajdova]

I can't see any real blockers at this moment. Can we move to a final review on this one? cc @siriwatknp

[mbrookes]

A couple of usability/accessibility issues:

• No keyboard focus indicator
• It isn't possible to escape from the editor with the keyboard; focus is trapped. Tab inserts tabs, Esc does nothing.

The error message could use more space between it and the ad below:

[nihgwu]

@mbrookes There is keyboard focus indicator, highlighted border, and Esc works to exit the editing mode

[mbrookes]

I'm definitely not seeing a keyboard focus indicator (highlighted border).

You're right, Esc does work, but it doesn't always return focus to the correct place. (It returns to the top if you hit Esc after you have typed something.) Previous iterations had the same issue though.

react-runner lacks this nice accessibility feature from Jarle. I've opened an issue.

[nihgwu]

@mbrookes I'm trying to address your latest feedbacks, and I noticed that the focus indicator is hardly noticeable in light mode, and I haven't came with a nice solution in this case

Regarding the a11y feature, actually it has nothing to do with react-runner itself, you can see in this PR, I'm using react-simple-code-editor directly

[mbrookes]

I noticed that the focus indicator is hardly noticeable in light mode

There's no keyboard focus indicator in dark mode either. It's the same whether focused with keyboard or mouse. (And looking at the code I see why. It isn't supported.)

In #24640:

Mouse:

Keyboard:

https://github.com/mui/material-ui/pull/24640/files#diff-4b49623cbefb6df3dac2ad985fd8d5ff5b4cac7f3b526397ad35d42cc80248c9R64-R78

@bharatkashyap may have iterrated on this in his attempts.

I'm using react-simple-code-editor directly

Ah, right. I'd forgotten that I had some logic to emulate the behaviour of Jarle: https://github.com/mui/material-ui/pull/24640/files#diff-4b49623cbefb6df3dac2ad985fd8d5ff5b4cac7f3b526397ad35d42cc80248c9R51-R62

[nihgwu]

@mbrookes pls check again

[mbrookes]

@nihgwu I can tab to it, but then I can't edit the content.

"pls check again" 😁

"Tab through" works, so that's progress, possibly... tab-through when it isn't possible to edit the editor content is effectively back to basics.

[nihgwu]

@mbrookes if you read the last commit you will find you can enter editing mode by press Enter, if you think it makes sense to support entering editing mode by any key, I'm fine to support it. And addressed other feedbacks in that change, so I don't think it's "effectively back to basics" 😉

[mbrookes]

if you read the last commit

I don't think most users would come and read the commit. 😁 The hint helps. Perhaps it should use aria-live?

if you think it makes sense to support entering editing mode by any key, I'm fine to support it

Since it's what the native textarea supports I think that's best. The behaviour of https://deploy-preview-30873--material-ui.netlify.app/components/buttons/ (which emulates Jarle) feels most natural to me.

@eps1lon Any thoughts on this?

[nihgwu]

I don't think most users would come and read the commit.

Sorry I mean reviewers

Since it's what the native textarea supports I think that's best.

But textarea doesn't capture Tab, personally I'd love the current behaviour, less mental burden on how tab works, but I'd say it's not critical for this PR, we can always improve it later, better performance, and better a11y support

[mbrookes]

Sorry I mean reviewers

The point being that, as implemented, the behaviour was not intuitive. If the only way to understand what's happening is to review the code, something's wrong. The Jarle-like hint added since definitely helps, but if we have to depend on it, it suggests that something still isn't quite right.

But textarea doesn't capture Tab

No, which can be a frustration in its own right, but I understand the tradeoff for a general text input. For code, the behaviour that allows Tab to be used for indents once editing has commenced, with Escape to break the focus trap makes perfect sense.

we can always improve it later, better performance, and better a11y support

The problem with "move fast and break things" is that someone has to remember to come back and fix them, especially when the person that broke them has moved on to other things. Since we already know what good looks like, let's do it right first time.