-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat(code): Add syntax highlighting as css (instead of inline styles in js) and explicitly import specific languages #75
Conversation
…languages we support
@@ -13,7 +13,6 @@ export default { | |||
output: { | |||
dir: 'lib', | |||
format: 'cjs', | |||
preserveModules: true, |
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.
@scurker this flag causes the build to break. For some strange reason, it emits lib/src
and lib/node_modules/highlight.js
. I don't know why we added it in the first place. Is it safe to remove?
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.
The old webpack code preserved the directory structure so I was trying to keep the same structure when migrating from webpack, so this option kept that output the same.
The only downside to not having it is debugging issues with cauldron, you'll see something like cauldron/lib/index.js:23484
instead of cauldron/lib/components/foo.js:234
. I'm not sure why it's just now causing issues but we can probably remove it.
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.
It's causing issues now because of how we're "registering" languages in the Code component. I don't fully understand it either 🤷
@@ -5,4 +5,4 @@ | |||
[[redirects]] | |||
from = "/*" | |||
to = "/index.html" | |||
|
|||
status = 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.
This fixes the "deployed" site, enabling it to act as a SPA. It should have been done in #76.
I know it's an unrelated change, but figured we didn't need a PR just for this.
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.
See comment about conflicting styles.
you got everything you need
<SyntaxHighlighter | ||
{...props} | ||
useInlineStyles={false} | ||
className={classNames('Code', className)} |
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.
👍
REVIEWED FOR SECURITY 🚓 🎨 |
Reviewer checks
Required fields, to be filled out by PR reviewer(s)
resolves #72
resolves #37