-
Notifications
You must be signed in to change notification settings - Fork 97
feat(typography)!: recolor CodeBlock and migrate to prism-react-renderer v2
#1864
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
Conversation
| const [isPrismImported, setIsPrismImported] = useState(false); | ||
| const win = useWindow(); | ||
|
|
||
| const importPrism = useCallback(async () => { | ||
| // TODO remove `importPrism` if/when `prismJS` releases v2 ESM | ||
| // https://github.com/orgs/PrismJS/discussions/3531 | ||
| if (win && !isPrismImported) { | ||
| (win as any).Prism = Prism; | ||
|
|
||
| await Promise.all([ | ||
| import('prismjs/components/prism-diff' as any), | ||
| import('prismjs/components/prism-json' as any) | ||
| ]); | ||
|
|
||
| setIsPrismImported(true); | ||
| } | ||
| }, [win, isPrismImported]); | ||
|
|
||
| useLayoutEffect(() => { | ||
| // Import languages missing from the vendored `Prism` using a variant of | ||
| // https://github.com/FormidableLabs/prism-react-renderer?tab=readme-ov-file#custom-language-support | ||
| // that is compatible with both React and Jest | ||
| importPrism(); | ||
| }, [importPrism]); |
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.
All of this goes away if/when PrismJS releases v2 with support for ESM imports 🙄
| <ThemeProvider | ||
| theme={parentTheme => ({ | ||
| ...parentTheme, | ||
| colors: { ...parentTheme.colors, base: isLight ? 'light' : 'dark' } | ||
| })} | ||
| > |
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.
Using the coerced mode theming technique here to render styled components based on the isLight prop.
| Prism={Prism} | ||
| code={code ? code.trim() : ''} | ||
| language={LANGUAGES.includes(language as Language) ? (language as Language) : 'tsx'} | ||
| isPrismImported && ( |
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.
Should we explore displaying a loading state while Prism deps are loading?
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 wonder if an enhancement could be made on this component (perhaps not right now) to allow a consumer-provided loading state.
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.
As this aims to be a temporary solution based on PrismJS's ESM roadmap, I hope we don't have to explore loading states.
| } | ||
| }, [win, isPrismImported]); | ||
|
|
||
| useLayoutEffect(() => { |
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.
[nit] Would useEffect be sufficient here?
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.
Also curious about this. My understanding is that useLayoutEffect is most useful in dealing with positioning and layout discrepancies. Does that apply for CodeBlock?
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.
useEffect is the right hook since we aren't rendering until language tokenization is in place. I've updated the code.
| await Promise.all([ | ||
| import('prismjs/components/prism-diff' as any), | ||
| import('prismjs/components/prism-json' as any) | ||
| ]); |
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.
Should we handle dynamic import failures with an error state?
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.
+1 to always have error states for dynamic imports
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.
We don't need an error state, per se. But we should always allow the component to render – whether or not it can be tokenized for language highlighting. The updated try .. catch .. finally allows for that.
|
I'm curious what the implication is for consumers/product implementations relying on the cut languages. Do we know who this affects and how wide spread it will be? Or is it a non-issue? |
geotrev
left a comment
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.
LGTM other than what is being discussed above.
| change: { hue: 'lemon', shade: 300 } | ||
| }[diff]; | ||
|
|
||
| backgroundColor = getColor({ theme, ...options, transparency: theme.opacity[200] }); |
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.
🔥
|
everything looks as expected on the design side! yay! |
@geotrev I searched all product code and the majority simply use |
|
@jzempel Awesome. Sounds like a minimal number of folks then. |
ze-flo
left a comment
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.
Awesome work! 💯
Description
Completed recolor work for
CodeBlockbothisLight={false}andisLight={true}.Detail
BREAKING CHANGE: the migration to
prism-react-renderercuts out a bunch of non-essential Prism languages.Checklist
npm start)renders as expected with reversed (RTL) direction?bedrock)tested in Chrome, Firefox, Safari, and Edge