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

feat(export): export pdf with outlines if --with-toc option is specified #669

Merged
merged 4 commits into from
Oct 28, 2022

Conversation

Shinyaigeek
Copy link
Contributor

Motivation 🔥

Fixes: #662

I implemented a feature to export pdf with outlines. A user can generate a pdf with outlines if they specify --with-toc option in command line.

I happened to see the issue, and it seemed interesting, so I started working on it, but it went better than I expected, so I'm submitting it as a PR. I would like to discuss the implementation policy using this branch as a starting point. Please feel free to comment on any concerns or overlooked points.

@antfu antfu requested a review from tonai July 31, 2022 21:51
const pdf = fs.readFileSync(output, { encoding: 'base64' })
const outline = slides.map(({ title }, idx) =>
title ? `${idx + 1}||${title}` : null,
).filter(outline => !!outline).join('\n')
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you want to manage the "deepness" of title ?
In that case you may also be interested in the level property.

Also please heck the following properties (rawTree, treeWithActiveStatuses and tree) from here https://github.com/slidevjs/slidev/blob/main/packages/client/logic/nav.ts#L79-L86
I think It would better to use the same logic and thus the same functions than the ones used for the <Toc> component
And then having a dedicated renderer for the outline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comment! I did not know <Toc /> component. I will refer it and change my logic to use same logic of <Toc />. However, I am wondering how to use rawTree in nodejs slidev because rawTree is plugged via a vite slidev plugin in client side build. 💭

Copy link
Contributor Author

@Shinyaigeek Shinyaigeek Aug 14, 2022

Choose a reason for hiding this comment

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

@tonai

I'm sorry for the late response 🙇

I changed to handle "deepness" of title. The current output is like the below.
スクリーンショット 2022-08-14 17 52 37

Also please heck the following properties (rawTree, treeWithActiveStatuses and tree) from here https://github.com/slidevjs/slidev/blob/main/packages/client/logic/nav.ts#L79-L86
I think It would better to use the same logic and thus the same functions than the ones used for the component
And then having a dedicated renderer for the outline.

I agree, but the current logic about <Toc /> is too for the client-side to share the logic also with export command in CLI…so I am wondering about design.

To share the logic about <Toc /> also with export command, I searched for a package to share the logic in the client-side and server-side( or command line side), but I could not find that… Do you know such a package for this use case?

Copy link
Contributor Author

@Shinyaigeek Shinyaigeek Aug 14, 2022

Choose a reason for hiding this comment

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

FYI: in the current implementation of this PR, the logic about ToC of PDF in export command is implemented adhocky separated from <Toc /> component's logic in client-side.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Shinyaigeek The idea of reuseing the same functions would be to ensure that the generated TOC will be the same as the one created with the <Toc/> component.
But yes those functions live in the client package.

@antfu what do you think of having a shared module that could be imported in either the backend or the frontend ?
Or should we duplicate some functions ? (here it is about all 3 addToTree, getTreeWithActiveStatuses and filterTree functions in client/logic/nav.ts)

Copy link
Contributor Author

@Shinyaigeek Shinyaigeek Aug 20, 2022

Choose a reason for hiding this comment

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

Yes, as @tonai says, it is better to share the logic which is reusable. If there is no package to share the utility with client-side and CLI, perhaps we should make such a package before promoting this PR.

@condorheroblog
Copy link

I want to say that @lillallol/outline-pdf is not perfect. The outline generated by @lillallol/outline-pdf can only jump to the corresponding page, but in the pdf editor, a good outline can jump to the corresponding part of the corresponding page.

image

@Shinyaigeek
Copy link
Contributor Author

@condorheroblog Thank you! I feel your concern will be resolved if @tonai’s advice is reflected.

@Shinyaigeek Shinyaigeek force-pushed the feature/export-pdf-with-toc-option branch from 7cde920 to ea873e9 Compare August 14, 2022 07:40
@Shinyaigeek Shinyaigeek requested a review from tonai August 14, 2022 09:00
@Shinyaigeek Shinyaigeek force-pushed the feature/export-pdf-with-toc-option branch 3 times, most recently from 0ef577a to cc6bd7d Compare August 14, 2022 11:36
@stale
Copy link

stale bot commented Oct 19, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 19, 2022
@Shinyaigeek
Copy link
Contributor Author

Not stale

@stale stale bot removed the stale label Oct 20, 2022
@antfu antfu changed the title Feature(slidev): export pdf with outlines if --with-toc option is specified feat(export): export pdf with outlines if --with-toc option is specified Oct 28, 2022
@antfu antfu merged commit d26139a into slidevjs:main Oct 28, 2022
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.

feature request: Export PDF to generate TOC(outline, bookmarks)
4 participants