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

Fix gatsby-remark-prismjs highlight bug #1921

Merged
merged 2 commits into from
Aug 29, 2017

Conversation

vincentbel
Copy link
Contributor

@vincentbel vincentbel commented Aug 26, 2017

Resolves #1917.

I use gatsby-dev-cli to test it in the demo repo, and it works now.

image

At first I forgot to clear the cache, and it drove me crazy. 😹

it(`also load the required language`, () => {
const language = `cpp`
const requiredLanguage = `c`
const Prism = require(`prismjs`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to isolate the prismjs module for each test so that tests won't affect each other. But it seems no easy way. any ideas? @KyleAMathews

Copy link
Contributor

Choose a reason for hiding this comment

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

Eh, dunno. Not enough of a Jest expert to know how to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

jest.resetModules()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KyleAMathews Works great! and it's ready to review now.

@gatsbybot
Copy link
Collaborator

Deploy preview ready!

Built with commit 67be0b6c40c6ca30fa2406cd8c1692c91a610b5b

https://deploy-preview-1921--using-drupal.netlify.com

@gatsbybot
Copy link
Collaborator

Deploy preview ready!

Built with commit 67be0b6c40c6ca30fa2406cd8c1692c91a610b5b

https://deploy-preview-1921--gatsbygram.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Aug 26, 2017

Deploy preview ready!

Built with commit f1b02d0

https://deploy-preview-1921--gatsbygram.netlify.com

@gatsbybot
Copy link
Collaborator

Deploy preview ready!

Built with commit 1f8240fec800f4e0f4516aa4b0805cad7ad0a98a

https://deploy-preview-1921--using-drupal.netlify.com

If language has dependencies, should load dependencies first.
e.g. cpp depends on c, should load c first.
@@ -1,5 +1,4 @@
const parseLineNumberRange = require(`../parse-line-number-range`)
const highlightCode = require(`../highlight-code`)

describe(`highlight code and lines with PrismJS`, () => {
it(`parses numeric ranges from the languages variable`, () => {
Copy link
Contributor Author

@vincentbel vincentbel Aug 29, 2017

Choose a reason for hiding this comment

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

@KyleAMathews Besides, I think these tests are some as:

it(`parses numeric ranges from the languages variable`, () => {
expect(parseLineNumberRange(`jsx{1,5,7-8}`).highlightLines).toEqual([
1,
5,
7,
8,
])
expect(parseLineNumberRange(`javascript{7-8}`).highlightLines).toEqual([
7,
8,
])
expect(parseLineNumberRange(`javascript{7..8}`).highlightLines).toEqual([
7,
8,
])
expect(parseLineNumberRange(`javascript{2}`).highlightLines).toEqual([2])
expect(parseLineNumberRange(`javascript{2,4-5}`).highlightLines).toEqual([
2,
4,
5,
])
})
it(`ignores negative numbers`, () => {
expect(parseLineNumberRange(`jsx{-1,1,5,7-8}`).highlightLines).toEqual([
1,
5,
7,
8,
])
expect(parseLineNumberRange(`jsx{-1..4}`).highlightLines).toEqual([
1,
2,
3,
4,
])
})
it(`handles bad inputs`, () => {
expect(parseLineNumberRange(`jsx{-1`).highlightLines).toEqual([])
expect(parseLineNumberRange(`jsx{-1....`).highlightLines).toEqual([])
})

Why duplicate here?

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's duplicates, feel free to PR them away!

@KyleAMathews
Copy link
Contributor

Thanks! Merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants