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

Support for fenced code block info string #345

Merged
merged 9 commits into from
Oct 6, 2018
Merged

Support for fenced code block info string #345

merged 9 commits into from
Oct 6, 2018

Conversation

arobase-che
Copy link
Contributor

@arobase-che arobase-che commented Jul 28, 2018

Hi \o

The discussion is on the corresponding issue #342

Spoiler: It's about info string ;)

💘

@wooorm
Copy link
Member

wooorm commented Jul 29, 2018

Build failed for some reason, I restarted it and now it’s green again!

@arobase-che
Copy link
Contributor Author

¯_(ツ)_/¯

@wooorm
Copy link
Member

wooorm commented Aug 19, 2018

@arobase-che Could you update the code here to match syntax-tree/mdast#22?

@arobase-che
Copy link
Contributor Author

arobase-che commented Aug 19, 2018

Humm, it's already done. See that comment. I updated both the same day (night ...).

@wooorm
Copy link
Member

wooorm commented Aug 19, 2018

@arobase-che Sorry, missed it! Soooo, I was in the process of merging this, but hmm, from what I read in the commonmark spec they call the whole thing the info string (I still like info instead of info string). However, as we’re removing the lang from it, I’m not sure info is still fitting. Would it make sense to use meta instead? I’m not sure where I saw it today, but I like it:

~~~js foo bar baz
   ^^^^^^^^^^^^^^ info
      ^^^^^^^^^^^ meta
   ^^ lang
console.log('!')
~~~

/CC @Hypercubed

@codecov-io

This comment has been minimized.

@ChristopherBiscardi
Copy link

Would love to see this get merged as we're getting requests for support in downstream projects (gatsby-mdx and mdx).

Is there anything I can do to help move this along? It looks like it might just need final review.

@arobase-che
Copy link
Contributor Author

Oups !
I forgot to update the mdast description ! It's now done.

Normaly after that, it will be merged soon. @wooorm is about to publish a new version.

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @arobase-che! 🙇

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

Hey this looks great! I refactored the code a bit so as to not use .trim() (which removed javascript white-space, which is not the same as markdown white-space). This unearthed that we needed some more fixtures (e.g., for multiple white-space, or for tabs), which I added.

This looks great, thanks @arobase-che!

@wooorm wooorm merged commit 31ef684 into remarkjs:master Oct 6, 2018
@wooorm
Copy link
Member

wooorm commented Oct 6, 2018

Landed!

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

Successfully merging this pull request may close these issues.

None yet

5 participants