Skip to content
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] Live editable demos #24640

Closed
wants to merge 17 commits into from
Closed

Conversation

mbrookes
Copy link
Member

@mbrookes mbrookes commented Jan 27, 2021

Some outstanding issues, but mostly dependant on upstream fixes, so ready for a first-pass review.

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

  • JS demo support.
  • TS demo support.
  • Keyboard focus indicator.
  • Enable copy button / code sandbox to use the edited version?
  • Fix whitespace that appears at the bottom of the page when the editor is focused.
  • Fix cursor placement when code expands (restricted by react-simple-code-editor. (Instead, don't expand the code on click.)
  • Refine accessibility (partly restricted by Jarle). (Fix merged upstream)
  • Console dependancy warnings from Jarle & TypeScript
  • Bonus: Parse the docs markdown to detect which demos need import files. (Currently creates imports for some non-editable demos.)
  • Fix -webkit-font-smoothing.
  • Fix server rendering of demos.
  • Lazy load the DemoEditor.
  • Enable demo file editing/HMR in development.
  • Fix HTML validation warnings in react-simple-code-editor.
  • The new dependencies aren't React v17 friendly (peer dependency)

Code:

For those keeping score, that's +~300 LOC (~100 of which are copied from another script, and might make sense to merge back).

@mbrookes mbrookes added the docs Improvements or additions to the documentation label Jan 27, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Jan 27, 2021

Details of bundle changes

Generated by 🚫 dangerJS against d53aba4

@eps1lon
Copy link
Member

eps1lon commented Jan 27, 2021

Do we really need to maintain 7000 new lines of code when codesandbox, stackblitz, jsbin and countless other solutions already exist?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 27, 2021

Do we really need to maintain 7000 new lines of code when codesandbox, stackblitz, jsbin and countless other solutions already exist?

The objective is to reproduce the DX you can get on:

I think that it's important. It's about having the inline previews being editable. Opening a CodeSandbox takes ages when you want to quickly test something (it breaks your flow).

@eps1lon
Copy link
Member

eps1lon commented Jan 27, 2021

@oliviertassinari You just had no argument in what you said. You just said: We need it because we need it. But why is missing as usual.

Note that only one of these examples is valid. 3 of the 4 do not apply because they don't allow one-click csb edit.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 27, 2021

@eps1lon The argument in #24640 (comment) was: CodeSandbox is slow to open. It's too slow when you need to quickly try something out, it breaks the flow. This is the current DX:

Jan-27-2021.12-20-01.mp4

@eps1lon

This comment has been minimized.

@eps1lon

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@mbrookes mbrookes marked this pull request as ready for review January 30, 2021 01:48
@mbrookes mbrookes changed the title [docs] WIP Live demos [docs] Live demos Jan 30, 2021
@oliviertassinari
Copy link
Member

Regarding make opening an external editor faster, I have experimented with https://www.skypack.dev/, but it's not here yet skypackjs/skypack-cdn#118.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 30, 2021

@mbrookes 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.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 31, 2021

Things I have noticed:

  • It's much better than what we had!
  • It's a great DX that Ctrl+A selects all the content of the demo and only (not the whole page). I'm so much more used to this approach. Another significant win.
  • The font is different to before. The change of -webkit-font-smoothing is questionable. I personally find it too crispy on macOS with antialiased. Codesandbox set it back to auto, we were using subpixel-antialiased before. Monaco Editor is also on auto.
  • The demos aren't rendered anymore on the server
  • We add 70 kB gzipped for each page. https://mui-dashboard.netlify.app/size-comparison?buildId=23398&baseRef=next&baseCommit=4eafaae996f250b8c4473c9be3f5637c22f28ef7&prNumber=24640. I wonder if it's expected, I would have expected some of the increase to be inside a shared chunk. We could lazy load the interactiveness of the demo, to make it a progressive enhancement. Right now, it's quite to may the price upfront +170%. .
  • We add HTML validation warnings. I had a look at them. They can all be fixed in react-simple-code-editor, but it will be some work.
  • The new dependencies aren't React v17 friendly (peer dependency)
  • We can't iterate on a demo anymore. 1. Editing the source doesn't live update 2. Editing the source with new imports crashes.

@oliviertassinari oliviertassinari marked this pull request as draft January 31, 2021 19:53
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 31, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 3, 2021

I now have write access to react-simple-code-editor which should allow releasing fixes upstream as needed.

@oliviertassinari oliviertassinari changed the title [docs] Live demos [docs] Live editable demos Feb 15, 2021
@o-alexandrov

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants