djm / remark-shortcodes

A custom Markdown syntax parser for remark that adds support for shortcodes.

MIT License

45 stars 7 forks source link

Add support for using shortcodes inline #6

Closed jeansaad closed 5 years ago

jeansaad commented 5 years ago

This resolves issue #5 and introduces a new option to allow shortcodes to be used inline without breaking the AST structure of using shortcodes within blocks. Happy to iterate!

codecov-io commented 5 years ago

Codecov Report

Merging #6 into master will increase coverage by 2%. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master     #6   +/-   ##
=====================================
+ Coverage      98%   100%   +2%     
=====================================
  Files           1      1           
  Lines          50     53    +3     
=====================================
+ Hits           49     53    +4     
+ Misses          1      0    -1

Impacted Files	Coverage Δ
index.js	`100% <100%> (+2%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5be2e6d...d43183c. Read the comment docs.

jeansaad commented 5 years ago

@djm Wondering if you could have a look at this at some point.

djm commented 5 years ago

Thank you for this! I haven't had a great deal of time for maintaining OS recently, so please excuse the lateness.

It looks great, and has tests & documentation - thank you very much for that.

I just have one question:

Does it allow block and inline to be used at the same time or is it a runtime choice as to which ones to look for? I haven't kept up with remark development so I'm a bit out of the loop here. I ask because if it doesn't look for both at the same time then "allowInline" doesn't really make sense as an option name.

jeansaad commented 5 years ago

So the implementation is either block or inline. It might be possible to get them to work together but I don't think that would be ideal since the AST could drastically differ and quite honestly it would be confusing as to the scenarios that a shortcode would be considered as a block over inline. Not saying it's impossible, but it could get tricky. Happy to take that route if we want to make it allow both though!

That being said, I agree that the naming of the option allowInline does seem like you can use both at the same time. Any thoughts on what to name this? We could call it inlineMode or inlineMethod?

jeansaad commented 5 years ago

@djm, I've renamed the variable to be more reflective of what it's doing. Let me know if this is ok with you.

djm commented 5 years ago

@jeansaad Looks great to me. Thanks very much for this change!

djm commented 5 years ago

Jean, I've added you to the contributors file and tagged v0.3.0. It should build now and auto-release to npm.

Thanks again for the help!

jeansaad commented 5 years ago

Thank you!