-
Notifications
You must be signed in to change notification settings - Fork 114
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
Unify code blocks into a single component #409
Conversation
@djsauble I would love a review from you on a few design related items. Specifically:
The dark mode feels great, but the light mode feels like it does not have enough contrast. Can you check this out and give some feedback here?
Now that the code block components are unified between the guides and reference documentation, we can enable things like the copy button and line numbers. Can you confirm these are the right choices?
Previously we were showing usage instructions using an inline style, which removed the padding and background, but kept the syntax highlighting. This has been switched to use the code block, which allows us the ability to add the copy button. |
src/components/MDXContainer.js
Outdated
code: ({ | ||
className, | ||
copy, | ||
lineNumbers, | ||
live, | ||
lineHighlight, | ||
preview: _preview, | ||
...props | ||
}) => ( | ||
<CodeBlock | ||
copyable={copy !== 'false'} | ||
highlightedLines={lineHighlight} | ||
language={className?.replace('language-', '')} | ||
lineNumbers={lineNumbers === 'true'} | ||
live={live === 'true'} | ||
{...props} | ||
/> | ||
), |
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.
Out of curiosity, why did you decide to destructure the props here rather than in the component? Is this so that we don't have to change the props being used in the .mdx
files?
That would make sense and, if that's the case, we might want to think about how we approach the problem of refactoring code without altering the content. Maybe not here and now, but we might want to add a layer to handle that formatting.
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'm not sure I follow what you're asking. Are you asking why I didn't put this logic inside of <CodeBlock />
itself?
I wanted <CodeBlock />
to have a more natural API since its used in more contexts than just MDX files. For example, I wanted to use the booleans true
and false
instead of their string representations. For the language, I felt it more natural to provide a language
prop rather than set as a className
. If I moved the logic inside of <CodeBlock />
, it would present an awkward API for usages outside of MDX.
To illustrate, here is what the API would look like in something like <ReferenceExample />
if we moved this logic inside of <CodeBlock />
itself.
<CodeBlock
copy="false"
live="true"
className="language-jsx"
/>
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 we're saying the same thing. You're manipulating the props here so that the <CodeBlock />
component can have a simpler API.
I kinda spaced out on the fact that we're using the <CodeBlock />
component in MDX and in JSX.
Either way, I do still think we should think about how we can refactor our components (and use our components in JSX) without needing to update how we use them in MDX. A discussion for another time, but something we will probably want to have a plan for.
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.
think about how we can refactor our components (and use our components in JSX)
To clarify, are you referring to other components and not just the CodeBlock
? The code block is a little bit of a special use case because it is generated using 3 backticks. For other components (like <Step />
), you can provide props to it like you would in JSX, which means we shouldn't need any special parsing for them:
These should all work in MDX:
<Step steps={4} onClick={() => console.log('clicked')}>
Some content for the step
</Step>
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 we're saying the same thing. You're manipulating the props here so that the component can have a simpler API.
And yes that is correct 😄 .
Technically speaking, I'm providing a function component inline here, I just didn't name it before assigning it to the code
key in the components
object. I could have just as easily done the following:
const MDXCodeBlock = ({ className, copy }) => (
<CodeBlock copyable={copy === 'true'} language={className.replace('language-', '')} />
)
const components = {
// ...
code: MDXCodeBlock
}
@@ -0,0 +1,10 @@ | |||
export const range = (a, b) => [...Array(b + 1).keys()].slice(a); |
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.
Super stoked to have a range
function here (why is this not part of JS in the first place?).
Would you be willing to add the ability to supply just one argument too? I could see benefit in doing stuff like range(5).map(n => console.log(n))
?
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.
Let's add that functionality when we have that use case. That complicates this function just a little bit, and we don't have a great reason to need a single argument range function quite yet. I do think its a perfectly valid idea.
|
||
const CodeBlock = ({ | ||
children, | ||
components: componentOverrides = {}, |
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.
Do we have any instances where we are adding components that aren't the Preview
? I'm a little unclear about the purpose of this prop.
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.
No. <Preview />
is the only component right now that is customizable. To understand how I got here, let me describe a few of my goals for this refactor and some ways I considered designing this API.
Goals
- We can use this anywhere in the site that needs a code block.
Regardless of where it is rendered, it should be able to handle all use cases demanded of the code block. This includes:
- conditionally showing line numbers
- conditionally allowing people to copy the code
- Allowing users to edit the code inline (useful for component docs)
- Show a live preview of the code snippet (also useful for component docs)
- Allow this component to be used to show live previews in guides
While this is explicitly disabled currently, its something we can enable when we have some demand for this particular use case.
- With the upcoming move to a shared theme, we should be able to cut/paste this component into the shared theme library and use it across all sites.
The API is designed to be used everywhere. You'll notice that there is no developer website specific code in here. We want to be able to use this component in the open source site as well, which means we have to try and be as generic as possible.
API Design
Goal 3 presents an interesting challenge. Because we want to enable live previews on any code on any site, not just the NR1 SDK in the developer site, we can't use something like the code in <ReferencePreview />
which uses shadow DOM, loads the NR1 SDK's CSS, etc. On the flip side, because we want to be generic about this, this also presents a problem for our use case of needing style isolation for the NR1 SDK component previews. This means that the live preview MUST be customizable.
Specifically, the NR1 SDK component previews require the following:
- Must be rendered in the shadow DOM to provide style isolation
- Must be able to load an external stylesheet
- Must be able to define inline CSS for additional styling (useful for guides like the
<Grid />
which show boxes) - Must be able to render other components (used to mount the
ToastManager
when theToast
component is used) - Must be able to define additional styles on the root preview container (useful for examples like the
<Spinner />
which renders absolutely positioned)
There are a couple of APIs I considered to allow this to be customizable with the previous requirements in mind:
- Allow extra props to be passed to configure the preview component:
<CodeBlock
preview={true}
previewExternalStylesheet="https://some.external.url/example.css"
previewInShadowDOM={true}
previewStyle={{ position: 'relative' }}
previewChildren={() => <ToastManager />}
>
import React from 'react'
// the rest of the code that should be highlighted by the code block
</CodeBlock>
While these may not have been the final names of the props, you can see that the surface area of <CodeBlock />
now has grown a lot, specifically with a LOT of props dedicated to configuring the preview. This approach also means we need to add new props any time we have new uses cases for the preview that weren't already considered.
- Use the
.
delimited approach to give back control of the structure to the user of the component
import root from 'react-shadow'
<CodeBlock>
<root.div>
<link href='stylesheet.css' />
<style type='text/css'>.h1 { font-weight: bold; }</style>
<CodeBlock.Preview />
</root.div>
<CodeBlock.Code>
import React from 'react'
// the rest of the code that should be highlighted by the code block
<CodeBlock.Code>
</CodeBlock>
While this approach is a bit better because I no longer have tons of preview
props dedicated to configuring the component preview, I've now lost control of the structure of <CodeBlock />
and can't enforce it. For example, what if someone uses the component this way?
<CodeBlock>
<CodeBlock.Code />
<CodeBlock.Preview />
</CodeBlock>
Do we honor this structure and render the preview underneath the code? And what about the status bar that renders the file name and copy button? Are those lost? Should we provide a component for that to make it obvious it's being rendered?
As you can see, while I gain the ability to easily customize the structure of the preview, I've now lost an enforced structure for the entire thing and put the burden back into the hands of the user of the component to always make sure these are in the proper order. I didn't like this unnecessary burden since we want an enforced structure for the code block.
- Allow a user to provide their own component for the preview
const MyCustomPreview = ({ children, className }) => (
<root.div>
<link href="some.css" />
{children}
</root.div>
)
<CodeBlock
preview={true}
components={{ Preview: MyCustomPreview }}
/>
This is popular with libraries like React Select and even the MDXProvider
for rendering mdx with customized components.
This approach allows us to be able to enforce the proper structure internally to the code block, but also provides the flexibility for the user to customize the structure of the preview component. This means users of the API can design the preview for any and all uses cases that may pop up over time without needing to touch the <CodeBlock />
to do so. This is the approach I opted for as I felt it provided the greatest flexibility without compromising the enforced structure we want in this component.
Final notes
Right now Preview
is the only component that is allowed to be customized because it is the only one that needs it. I'm not sure it makes sense to provide a customized status bar, or customized code highlighting as of right now. If that use case ever exists, then we have a way to extend this component to allow for those customizations.
Hope that explains the reasoning behind this API design choice and why I chose to go the way I did.
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.
🥇
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Closes #165
Closes #136
Closes #375
Description
Unifies the code blocks into a single component. Currently there are 3 variants of a code block:
<CodeSnippet />
used for guides,<ReferenceExample />
used for component previews/live editing, and<InlineCodeSnippet />
used for component usage instructions. This has been unified into a single<CodeBlock />
component that can handle line numbers, live editing, component previews, code formatting, etc. This refactor enables line numbers and the copy button for component and API docs.On top of the unification, the Nord theme has been added to match the theme used on the open source site. Both a dark and light variant has been added for light and dark modes.
This PR also disable line numbers by default.
Screenshot(s)
Before


After

Guide template
