-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
SyntaxHighlighter: Expose registerLanguage #23166
Conversation
@hobbes7878 @timbomckay @marcosmko @jnschrag You all indicated a desire to be able to inject additional languages into storybook's SyntaxHighlighter. |
I tried to give this a test following the Contributing guidelines to run a sandbox template on this branch and am getting this error:
This is
And I added this to
I'm sure I'm likely doing something incorrectly in setting this up, so will be interested to see if others are able to test successfully. In theory, this seems like it should work. |
@jnschrag I appreciate you testing this out.. I found that due to us lazy-loading the syntax-highlighter (due to it's size) I wasn't passing the API though deep enough. I made another attempt. Would you care to give this one a try? |
It's no longer giving me any errors, but it doesn't seem to actually be applying the formatting either. I added a
Let me know if I can provide any other information to help debug this. |
Hi. Thanks for getting this started @ndelangen. I had the same result as @jnschrag, but I've dug around a bit and think I may have a clue. For MDX docs, the storybook/code/ui/components/src/syntaxhighlighter/lazy-syntaxhighlighter.tsx Lines 36 to 40 in 711f2ea
... always returns Trouble is, only storybook/code/ui/components/src/syntaxhighlighter/lazy-syntaxhighlighter.tsx Lines 9 to 32 in 711f2ea
If, for example, I reverse the condition based on Upshot: I think if we include the same language registering logic in both the formatted and non-formatted lazy-loaded UPDATE: Added a PR to this branch that takes the shortest route to duplicate the logic for |
@hobbes7878 I've invite you to the github org, if you have interest, could you make your changed this this PR? |
Done and verified working in MDX docs. I do see tests are failing on a missing type for the new method:
|
This reverts commit 3975d93.
Messed with the type error above, which comes from: storybook/code/ui/blocks/src/components/Source.tsx Lines 8 to 24 in 602a01a
If I retype and cast import type { SyntaxHighlighterProps } from '@storybook/components';
const StyledSyntaxHighlighter: React.FunctionComponent<SyntaxHighlighterProps> = styled(SyntaxHighlighter)//... ... I seem to pass type check. Happy to push that change @ndelangen, but there may be a better way... |
@hobbes7878 I think it'd be awesome if we could add a story that demonstrated adding a custom syntax being injected. Having a story demonstrates the behavior as well as serve as a test, because chromatic will apply visual regression and we'll be alerted if we'd ever regress. Would you be able to add such story? I personally don't really have a preference for how to do the types.. perhaps @kasperpeulen has? @kasperpeulen please have a look at the changes in this PR and check if you would want any changes to the way types get defined etc. 🙏 |
Apologies, @ndelangen, was out of town a few days. I'm happy to take a swing at documenting the change with a new story. Always figuring out how to work with the monorepo takes the longest, so if you can point me to a similar story that makes changes to storybook's overall config, i.e., |
@hobbes7878 thank you so much for all the effort! Here's a place in the code where we might add such story: storybook/code/ui/components/src/syntaxhighlighter/syntaxhighlighter.stories.tsx Lines 133 to 158 in 1e9e0c4
My idea would be to a a new story below and similarly using a custom render function. Alternatively, we could also use a custom loader function? 🤔 Whichever you prefer. |
// Register custom language | ||
SyntaxHighlighter.registerLanguage('scss', scss); |
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 think this should be done, as the story renders, a custom render function would allow us to do that.
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.
Ah, gotcha. Let me try that. 👍
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.
Realize I'm reading too quickly and did this in a custom loader instead of render func in a9d0d22. The loader works, but let me know if you'd prefer I go the render route. Happy to rewrite.
code/ui/components/src/syntaxhighlighter/syntaxhighlighter.stories.tsx
Outdated
Show resolved
Hide resolved
code/ui/components/src/syntaxhighlighter/syntaxhighlighter.stories.tsx
Outdated
Show resolved
Hide resolved
Sadly I can't approve the PR.. because.. I made it 😄 This LGTM, let's wait for the CI to approve as well! 👏 |
Thank you all so much for your work on this! It's going to be so helpful for my team. |
Closes #21341
What I did
I exposed a method for registering new languages for our syntax highlighter.
I'm unsure if registering new languages after storybook rendered will actually work, but maybe we can inject early enough, or maybe it does just work.
How to test
We'd need to figure out a place where this can be added.
The SyntaxHighlighter can be rendered in docs, where it's most likely this feature-request is wanted.
So likely we'll need to experiment with adding something to
.storybook/preview.ts
.Perhaps this would work:
Checklist
If verified it actually work, or tweaked to make it work, we should ensure this behavior is tested, possibly in a story.
Maintainers
make sure to add the
ci:merged
orci:daily
GH label to it.["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]