[beta] upgrade sandpack and refactor sandpack.css #4843
Merged
Conversation
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
danilowoz
commented
Jul 27, 2022
| @@ -1,37 +0,0 @@ | |||
| diff --git a/node_modules/@codesandbox/sandpack-react/dist/esm/index.js b/node_modules/@codesandbox/sandpack-react/dist/esm/index.js | |||
There was a problem hiding this comment.
Sandpack@1.x already supports React 18 templace
danilowoz
commented
Jul 27, 2022
|
|
||
| {showDevTools && ( | ||
| <SandpackReactDevTools onLoadModule={onDevToolsLoad} /> | ||
| <SandpackLayout |
There was a problem hiding this comment.
SandpackLayout pretty much does the same thing of the previous code
danilowoz
commented
Jul 27, 2022
| </SandpackLayout> | ||
|
|
||
| {showDevTools && ( | ||
| // @ts-ignore TODO(@danilowoz): support devtools |
There was a problem hiding this comment.
TODO to be addressed after this PR: reimplement react-devtools-inline support
There was a problem hiding this comment.
Shall we create an issue so that we don't miss this one?
There was a problem hiding this comment.
I have a PR waiting to be merged, once we merge this one.
danilowoz
commented
Jul 27, 2022
| import cn from 'classnames'; | ||
| import {Error} from './Error'; | ||
| import {SandpackConsole} from './Console'; | ||
| import type {LintDiagnostic} from './useSandpackLint'; | ||
|
|
||
| /** | ||
| * TODO: can we use React.useId? |
Size Changes📦 Next.js Bundle AnalysisThis analysis was generated by the next.js bundle analysis action 🤖 🎉 Global Bundle Size Decreased
DetailsThe global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster. Any third party scripts you have added directly to your app using the If you want further insight into what is behind the changes, give @next/bundle-analyzer a try! Two Hundred Thirty-one Pages Changed SizeThe following pages changed size from the code in this PR compared to its base branch:
DetailsOnly the gzipped size is provided here based on an expert tip. First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If Any third party scripts you have added directly to your app using the Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 10% or more, there will be a red status indicator applied, indicating that special attention should be given to this. |
danilowoz
commented
Jul 27, 2022
| autorun, | ||
| initMode: 'user-visible', | ||
| initModeObserverOptions: {rootMargin: '1400px 0px'}, | ||
| bundlerURL: 'https://6b760a26.sandpack-bundler.pages.dev', |
There was a problem hiding this comment.
I didn't update the bundler in order to avoid unnecessary changes, but I'd recommend upgrading it later.
danilowoz
commented
Jul 27, 2022
| @@ -10,7 +10,6 @@ import '@docsearch/css'; | |||
| import '../styles/algolia.css'; | |||
| import '../styles/index.css'; | |||
| import '../styles/sandpack.css'; | |||
| import '@codesandbox/sandpack-react/dist/index.css'; | |||
danilowoz
commented
Jul 27, 2022
|
|
||
| const MyDocument = () => { | ||
| // @todo specify language in HTML? | ||
| return ( | ||
| <Html lang="en"> | ||
| <Head /> | ||
| <body className="font-sans antialiased text-lg bg-wash dark:bg-wash-dark text-secondary dark:text-secondary-dark leading-base"> | ||
| <style id="sandpack">{getSandpackCssText()}</style> |
There was a problem hiding this comment.
This will inject the CSS variables, and later the components styles after Suspense rendering Sandpack components
danilowoz
commented
Jul 27, 2022
| @@ -2,289 +2,268 @@ | |||
| * Copyright (c) Facebook, Inc. and its affiliates. | |||
There was a problem hiding this comment.
Most of the changes from this PR are on this file. Here are the highlights of these changes:
- Removed unnecessaries
!important; - Tried to reuse tailwind variables instead of hardcoded colors;
- Addressed breaking changes due to the new Sandpack version;
- Reorganized and regrouped styles by categories;
- Kept the "See more" section as it was; there was no need to changes it;
danilowoz
commented
Jul 27, 2022
| @@ -467,6 +479,19 @@ | |||
| "@codemirror/view" "^0.19.0" | |||
| "@lezer/javascript" "^0.15.1" | |||
|
|
|||
| "@codemirror/lang-markdown@^0.19.3": | |||
There was a problem hiding this comment.
I don't know if it's useful, but from now on, Sandpack supports plain text and markdown files
|
@harish-sethuraman, I'd appreciate it if you could help out by reviewing this. I know it's a big PR, but you can navigate through commits, then make it'll easier to understand all changes. Plus, after merging this PR, I believe we can reduce the first-load bundle size even more. |
|
Screen.Recording.2022-07-28.at.11.35.23.AM.movThere is a difference in background color when we shift to mobile view |
|
There a also seems to be a little bit different in sandbox sizes for laptop devices probably there is a change in font
Seems like when a line is highlighted for error we introduced a border radius there as well. Not sure if that was intentional or not :) These are the changes I noticed between main and this branch |
|
harish-sethuraman
approved these changes
Jul 28, 2022
gaearon
reviewed
Jul 29, 2022
gaearon
requested changes
Aug 4, 2022
There was a problem hiding this comment.
On a long page like https://beta-reactjs-8lyhiud9b-fbopensource.vercel.app/learn/thinking-in-react, when you "show more" and expand the sandbox, the preview area should be sticky. I don't think this works now.
|
✅ Sticky long sandboxes; |
|
If you turn on "highlight updates", there's something weird happening with preview re-renders. I think it constantly re-renders every example in a loop. |
|
Hey, @gaearon I couldn't reproduce it. Is it happening in a specific sandbox or all of them? Screen.Recording.2022-08-08.at.09.40.42.mov |
|
Can't repro either. Not sure what this was. Let's give it a try? |
|
I think this added a development regression. When I edit regular code blocks, Fast Refresh seems to no longer (or at least not consistently) pick up my edits. |
gaearon
added a commit
that referenced
this pull request
Sep 2, 2022
|
Reverted in #4959 (comment) |
|
wait this one has colors in SSR?????!!! |
gaearon
added a commit
that referenced
this pull request
Sep 10, 2022
|
merged via #5033 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A new Sandpack version has been shipped, which contains a ton of improvements to how we manage the styles and make the API more consistent. So this PR upgraded Sandpack and took the change to refactor some parts that weren’t easy to maintain, especially the
sandpack.cssfile.Why now? React Docs is currently using a different channel (
@codesandbox/sandpack-react@experimental), which was introduced due to some hard dependencies fromreact-devtools-inline. However, over time it started to be very out date from themainSandpack branch, which contains the new features mentioned above, and doing this migration will make it easier to introduce future feature requests and bug fixes.What are the risks? As you can see, most of the changes are API breaking changes and the CSS file refactor. I’m pretty confident that the new Sandpack version will not introduce any problem because it’s already running for a few months without any problem. Despite the many tests I did, the style changes might introduce some regression due to the number of changes in this file. But, in order to avoid any visual regression, I did the following testing routine:
TODO / known issues
react-devtools-inlineis still depending on some unpublished React API (confirmed with Brian feat(react-devtools): update to react-devtools-inline codesandbox/sandpack#537), so after merging this PR, I need to create an updatedexperimentalchannel on Sandpack in order to support it again. Hopefully, it’s fine to merge this PR without React Devtools because currently, there’s no playground with this component.Footnotes
Visual regression tests: left side (main branch), center (current branch), and the right one is the visual difference between them. You can note that there in the NavigationBar component, there is a typo (
foooo) that works with the purpose of making sure the visual testing is working. Plus, there are some pages that have some unexplained visual differences (rendering issue?), which can be discarded as many others pages don't contain the same problem. ↩Regression checklist
- Decorations are still rendering (
- It’s possible to run multiple DevTools on the same page;
- Edit content on markdown file;
- Reset button: it should reset the content and refresh the preview;
- The line-height of the CodeEditor component is correctly
- Navigate in the codebase (command + arrow keys);
- Scroll tests: jump to the very end of the page, go back to a Sandpack;
- On loading state: no layout shift between placeholder and editor;
- Mobile version: no layout shift on the scroll;
- Show inline error;
- No extra console logs;
- Make sure all snippets code are really read-only (focus & tab, command + X);
↩/apis/usestate);24px;