Skip to content
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

Merged
merged 16 commits into from
Aug 27, 2020

Conversation

schne324
Copy link
Member

@schne324 schne324 commented Aug 25, 2020

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Tested for accessibility
  • Code is reviewed for security

resolves #72

resolves #37

@@ -13,7 +13,6 @@ export default {
output: {
dir: 'lib',
format: 'cjs',
preserveModules: true,
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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
Copy link
Member

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.

Copy link
Member

@stephenmathieson stephenmathieson left a 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.

packages/styles/code.css Outdated Show resolved Hide resolved
@schne324 schne324 dismissed stephenmathieson’s stale review August 27, 2020 17:15

you got everything you need

<SyntaxHighlighter
{...props}
useInlineStyles={false}
className={classNames('Code', className)}
Copy link
Member

Choose a reason for hiding this comment

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

👍

@stephenmathieson
Copy link
Member

REVIEWED FOR SECURITY 🚓 🎨

@schne324 schne324 merged commit 2dd315b into develop Aug 27, 2020
@schne324 schne324 deleted the code-component branch August 27, 2020 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants