Skip to content

Conversation

jzempel
Copy link
Member

@jzempel jzempel commented Jul 18, 2024

Description

Completed recolor work for CodeBlock both isLight={false} and isLight={true}.

Detail

BREAKING CHANGE: the migration to prism-react-renderer cuts out a bunch of non-essential Prism languages.

Checklist

  • 👌 design updates will be Garden Designer approved (add the designer as a reviewer)
  • 🌐 demo is up-to-date (npm start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
  • ♿ tested for WCAG 2.1 AA accessibility compliance
  • 📝 tested in Chrome, Firefox, Safari, and Edge

Comment on lines 48 to 71
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]);
Copy link
Member Author

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 🙄

Comment on lines +107 to +112
<ThemeProvider
theme={parentTheme => ({
...parentTheme,
colors: { ...parentTheme.colors, base: isLight ? 'light' : 'dark' }
})}
>
Copy link
Member Author

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 && (
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member Author

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(() => {
Copy link
Contributor

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?

Copy link
Contributor

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?

Copy link
Member Author

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.

Comment on lines 57 to 60
await Promise.all([
import('prismjs/components/prism-diff' as any),
import('prismjs/components/prism-json' as any)
]);
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Member Author

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.

@geotrev
Copy link
Contributor

geotrev commented Jul 19, 2024

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?

Copy link
Contributor

@geotrev geotrev left a 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] });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@sieeeeeenna
Copy link

everything looks as expected on the design side! yay!

@jzempel
Copy link
Member Author

jzempel commented Jul 19, 2024

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 I searched all product code and the majority simply use <CodeBlock> without language specified (defaults to tsx). What we have here is already more extensive than what is currently in use. And I've added bash back in order to retain support for developer API docs.

@jzempel jzempel requested review from geotrev and ze-flo July 19, 2024 21:42
@geotrev
Copy link
Contributor

geotrev commented Jul 19, 2024

@jzempel Awesome. Sounds like a minimal number of folks then.

Copy link
Contributor

@ze-flo ze-flo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! 💯

@jzempel jzempel merged commit 5fd3fd9 into next Jul 22, 2024
@jzempel jzempel deleted the jzempel/recolor-codeblock branch July 22, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants