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

WIP - Add table of content option and generation #75

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

scandinave
Copy link

PR for #74

@scandinave
Copy link
Author

scandinave commented Jun 17, 2020

You can follow the progression .

Should I provide a default theme for the content table or let the user provide his own?

@scandinave
Copy link
Author

I just test my implementation with page break. All headings after page break are not take into account. Any idea to fix this?

@scandinave
Copy link
Author

Ok, this is corrected. Just one test that failed.

test('getHtml should inject rendered markdown', (t) => {
	const html = getHtml('# Foo', defaultConfig).replace(/\n/g, '');

	t.regex(html, /<body class=""><h1 id="foo">Foo<\/h1>.*<\/body>/);
});

I don't understand how this can be validated as getHtml always return string with tag. any idea?

Copy link
Owner

@simonhaenisch simonhaenisch left a comment

Choose a reason for hiding this comment

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

Just while you're at it, I'm not really a fan of OOP, so can you change the InitMarked class back to a function instead? It would be nice if the TOC generation function is a separate module and we can load it with marked.use().

Another thing we need to do is check !Object.prototype.hasOwnProperty.call(renderer, 'heading') because we don't want to override a user's custom heading renderer.

Also, I find the current configuration too complicated. It should just check sth like

html.includes('<!-- TOC -->')

and if present, it should automatically be enabled. Then toc_heading and toc_depth can be two new config options (not nested).

Alternatively, we could actually go with <div class="toc"></div> because then we could allow custom content there, like

<div class="toc">

## Table of Contents

</div>

(the blank lines are important for this mix of html and markdown btw)

and then append the TOC. More complicated to do this though (as we can't use document.querySelector, unless we only append the TOC in the browser context).

@scandinave
Copy link
Author

Ok i will update ma PR soon. The type definition for Marked is out of date and missing use function.

@simonhaenisch
Copy link
Owner

Cool thanks! You can do (marked as any).use(...) for now, and hopefully someone will update @types/marked soon 🤓

@scandinave
Copy link
Author

I have made a PR on definitlytyped to update the typings.

@scandinave scandinave force-pushed the master branch 2 times, most recently from a9fe4fc to 61cb8fd Compare June 19, 2020 08:04
@scandinave
Copy link
Author

Ok i have make the change. but there is still the test 'getHtml should inject rendered markdown' that does not pass

readme.md Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
src/lib/config.ts Outdated Show resolved Hide resolved
src/lib/get-html.ts Outdated Show resolved Hide resolved
src/lib/get-html.ts Outdated Show resolved Hide resolved
src/lib/get-marked-with-highlighter.ts Show resolved Hide resolved
@@ -1,20 +1,18 @@
import { getLanguage, highlight } from 'highlight.js';
import marked, { MarkedOptions } from 'marked';

export const getMarked = (options: MarkedOptions) => {
export const getHighlightRenderer = (options: MarkedOptions) => {
const renderer = options.renderer ?? new marked.Renderer();
Copy link
Owner

Choose a reason for hiding this comment

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

It doesn't make sense to have this in combination with marked.use() anymore. If there's options.renderer, then we need to call marked.use({ renderer: options.renderer }) instead.

Copy link
Author

Choose a reason for hiding this comment

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

can you explain me a bit more ? i'm not see what modification i need to do.

export const getHeadingRenderer = (config: Config) => {
toc = [];
const options = config.marked_options;
const renderer = options.renderer ?? new marked.Renderer();
Copy link
Owner

Choose a reason for hiding this comment

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

Same thing as previous comment, we don't want to extend the user's custom renderer.

@scandinave
Copy link
Author

Great news. My PR for the type was accepted . DefinitelyTyped/DefinitelyTyped#45578

@simonhaenisch
Copy link
Owner

simonhaenisch commented Jun 25, 2020

Hey @scandinave I've gotten busy with other work and won't be able to have a proper look at this in the next couple weeks. Are you ok to just use your fork in the mean-time? You can npm uninstall -g md-to-pdf and then npm link in your fork, so that it's symlinked into your global registry.

I'll get to it when I have some spare time for this project again.

@scandinave
Copy link
Author

Ok no problem. Take your time :)

@MangelMaxime
Copy link

Hello,

I wonder what will happen for people using an extension to manage their TOC.

Like Markdown TOC in VSCode.

<!-- TOC -->

- [Title 1](#title-1)
    - [Title 1.1](#title-11)
- [Title 2](#title-2)
- [Title 2.2](#title-22)

<!-- /TOC -->

# Title 1

## Title 1.1

# Title 2

# Title 2.2

I suppose you are using <!-- TOC --> presence to trigger the TOC generation and so in this case the output file with have 2 TOC in it.

If that's the case, I think it would be nice to add a new options to control if TOC should be generated or not and use <!-- TOC --> to know the position where the TOC should be placed in the file.

@simonhaenisch
Copy link
Owner

Could just use --toc-depth=0 to disable it, I guess.

@scandinave
Copy link
Author

--toc-depth=0 will only makes links to not be generated. But the wrapper's title of the TOC will still be generated.

I can just add a check for toc-depth=0 in this condition if (html.includes('<!-- TOC -->') && toc.length !== 0) { to fix this or add another parameter as suggested by @MangelMaxime.

@mr-potato-head
Copy link

Any news on the TOC feature ?

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

4 participants