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

use headerIds to remove header IDs #144

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

use headerIds to remove header IDs #144

wants to merge 2 commits into from

Conversation

wellingguzman
Copy link

Instead of overrides marked's default header with IDs using the renderer heading function, it uses the headerIds option added in version 0.4.0.

It also pass down the options parameter to marked to add the ability to set the headerIds or any options to marked.

@misterhtmlcss
Copy link
Collaborator

Hi @wellingguzman thank you for your submission. Can you please run the tests, find which tests need to be altered and then make those changes inclusive with your PR?

Also as of this writing there is a relevant update to this issue coming: markedjs/marked#1401 and it looks as though we'll need to wait and first decide if we are going to bump our markedjs version and then if so we'll want to roll in your changes at that time. Does that make sense? Don't want to introduce potential bugs and breaking changes without first having seen those features properly resolved upstream from us.

Super appreciate your contribution!

@wellingguzman
Copy link
Author

I ran the tests, they all passed. I believe this line confirms there's not IDs in the header.

At the moment, there's no need to bump the version of marked, as it's already included in the version terraform uses, and because there's no way to enable headingIds through terraform (based on this line).

I believe this is safe to merge without any introducing potential bug, as this is only disabling it using the options instead of overrides the renderer function.

@wellingguzman
Copy link
Author

I also removed the options from the marked to make this more likely to not be change at the moment. Either way this parameter was not intended to be marked options.

After markedjs/marked#1401 is fixed/implemented, enabling headingIds would be ideal.

@wellingguzman
Copy link
Author

wellingguzman commented Jan 8, 2019

@misterhtmlcss it's worth to mention that they released 0.6.0 hours after your comment.

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.

2 participants