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

Added support for a cover page when generating a PDF (#2004) #4342

Merged
merged 9 commits into from Apr 12, 2019

Conversation

icnocop
Copy link
Contributor

@icnocop icnocop commented Apr 9, 2019

No description provided.

@bitbonk
Copy link
Contributor

bitbonk commented Apr 10, 2019

Usually (for us) a cover page does not contain a lot of text but is more of a page with special layout, e.g. A big title centered in the middle, a smaller subtitle, a company logo, no footers no headers, maybe a date and copyright notice in a small special font.

So the markdown is not really the problem. The bigger problem is: How can I define that special layout for the cover page alone? How can I have footers or headers with page numbering on all of my pages but not on the cover page (and the ToC pages)?

Does this PR make those things easier or even possible?

@icnocop
Copy link
Contributor Author

icnocop commented Apr 10, 2019

Hi @bitbonk.

I think footers, headers, page numbers, and having the ability to provide a different layout for different pages are different issues.

@superyyrrzz
Copy link
Contributor

superyyrrzz commented Apr 10, 2019

I think one point from @bitbonk is that is is not flexible to use a Markdown file to define a cover page. IMO, a cover page is like some title in the middle of a page, but we can only get an upper-left H1 header by a Markdown input.

With this, I'd prefer a _cover.pdf instead of cover.md. DocFX can insert this PDF at the start of generated PDF, and users can put whatever in this one page _cover.pdf.

Of course I rarely use this PDF feature, so I'd like to here your thoughts. @bitbonk @icnocop

@icnocop
Copy link
Contributor Author

icnocop commented Apr 10, 2019

@bitbonk mentioned

the markdown is not really the problem

I understood that as "Markdown is fine." Was I mistaken?

In any case, markdown is flexible enough so we're not limited to only an upper-left H1 header.

For example, this markdown file will generate a page with "Cover Page Title" in the middle of the page:

<style>
.cover-title {
    height: 200px;
    width: 400px;
    position: fixed;
    text-align: center;
    top: 50%;
    left: 50%;
    margin-top: -100px;
    margin-left: -200px;
}
</style>
<div class="cover-title">Cover Page Title</div>

With these changes, if you name the file cover.md and include it in docfx.json as content, it will be added as the first page in the generated PDF.

Requiring a PDF file as the cover page doesn't seem very flexible or document-author-friendly to me.
It introduces another file format as input which is harder to diff and modify than the markdown and yaml files currently being used as inputs for conceptual documentation.

@icnocop
Copy link
Contributor Author

icnocop commented Apr 10, 2019

In regards to changing page margins, I think I agree with @PhilterPaper's comment here: wkhtmltopdf/wkhtmltopdf#3635 (comment)

To remove or change margins on an arbitrary interior page would probably better be done with embedded directives (special HTML comments)

In that way, each page could have it's own margins defined from within the page itself.

We may want to also think about that approach for overriding the headers and footers in specific pages for example.

Instead of HTML comments, it may be better if they were "markdown comments".
For example, "markdown comments" can be written like the examples here: https://stackoverflow.com/a/20885980/90287.
Microsoft.DocAsCode.HtmlToPdf.dll would probably have to parse the files to extract those directives though.

So basically, instead of littering docfx.json with options for each specific page, options could be defined within the pages themselves to override the global/default options.

@icnocop
Copy link
Contributor Author

icnocop commented Apr 10, 2019

Another approach other than using "markdown comments" is to have an accompanying JSON or YAML file for each markdown file that contains these properties.

For example cover.md.json:

{
  margin-bottom: 0,
  margin-left: 0,
  margin-right: 0,
  margin-top: 0,
  orientation: "Landscape",
  footer-center: "",
  footer-left: "",
  footer-right: "",
  header-center: "",
  header-left: "",
  header-right: ""
}

Those options would correspond to wkhtmltopdf's page, header, and footer options.
https://wkhtmltopdf.org/usage/wkhtmltopdf.txt

However, wkhtmltopdf doesn't currently support setting different margins and orientation for different pages.
See wkhtmltopdf/wkhtmltopdf#3635 and wkhtmltopdf/wkhtmltopdf#1564

@superyyrrzz
Copy link
Contributor

superyyrrzz commented Apr 10, 2019

@icnocop Thank you for your thoughts. It convinced me that PDF is not an ideal cover input.

My other concern is whether we need cover.md. If it is built into HTML by docfx, put to output folder and added to manifest.json, that means cover.html is part of the output that users can view final site. However, cover.html should just be an intermediate output used by PDF pipeline. It should be excluced from output.

Inspired by your example, I think _cover.html could be a purer input. wkhtml can use it as input directly, without need to borrow Markdown pipeline and pollute HTML output. It is more straightforward for cover page author to write styles in HTML than in Markdown.

I still have no ideas about the margin issue. I agree this could be another separate topic.

@bitbonk
Copy link
Contributor

bitbonk commented Apr 10, 2019

I think I would like it better if I just could provide a cover.html (that uses the styles of my PDF DocFX template) or even a cover.pdf instead of cover.md.

There wouldn’t be much text I‘d add to a cover page, hence the cover.md is not very useful. In my use cases, the cover page is more about layouting and less about (text)content.

@icnocop
Copy link
Contributor Author

icnocop commented Apr 10, 2019

My other concern is whether we need cover.md. If it is built into HTML by docfx, put to output folder and added to manifest.json, that means cover.html is part of the output that users can view final site.

I think it should be recommended that cover.md be placed in the pdf folder, like the toc.yml file specific for PDF.
https://dotnet.github.io/docfx/tutorial/walkthrough/walkthrough_generate_pdf.html#step1-add-a-tocyml-specific-for-pdf

In this way, the generated cover.html isn't part of the generated web site, and only part of the generated PDF.

Inspired by your example, I think _cover.html could be a purer input. wkhtml can use it as input directly, without need to borrow Markdown pipeline and pollute HTML output. It is more straightforward for cover page author to write styles in HTML than in Markdown.

I think I would like it better if I just could provide a cover.html (that uses the styles of my PDF DocFX template)

If you start from scratch, using the default PDF template, and want to create an HTML file for the cover page, you don't have access to the CSS styles as part of the template; you'll have to extract the default pdf template first in order to use the CSS styles which are part of the template in order to create a cover page in HTML.

Using a cover.md file resolves that requirement because it's processed by the pipeline and the cover HTML file is generated with the CSS styles included from the template.

This is more in-line with how other conceptual documentation files are processed through the pipeline - using a markdown file.

If you want to use HTML files as a type of input, it seems like that HTML file should be part of the template, where other HTML and CSS files exist as part of the pipeline.

@superyyrrzz
Copy link
Contributor

superyyrrzz commented Apr 11, 2019

I think it should be recommended that cover.md be placed in the pdf folder, like the toc.yml file specific for PDF.

Agree to make this as a convention. You are right that we need not care about site or manifest things here.

If you want to use HTML files as a type of input, it seems like that HTML file should be part of the template, where other HTML and CSS files exist as part of the pipeline.

👍 I like this idea. The detailed design in DocFX template system could be:

  1. user should prepare an input cover.md that specifies to use template unique for cover page:

    ---
    documentType: cover
    ---
    # The real cover page title
  2. Add pdf.default/cover.html.primary.html, which can contain default styles for cover page.

  3. PDF coomand to combine them.

With the default cover page template, user can define cover page easily. If user has many custom style needs as @bitbonk mentinoed, go to custom cover.html.primary.tmpl.

@icnocop
Copy link
Contributor Author

icnocop commented Apr 11, 2019

@superyyrrzz , thank you very much for your detailed review.

I have resolve your review comments in the latest commits as part of this PR.

I also added support for using cover.html.primary.tmpl from your suggestion and added it to the default pdf template.

For now, the template simply renders the text as in your sample cover.md in the middle of the page.

However, I didn't understand what you meant by:

PDF [command] to combine them

Can you clarify that for me please?

Thank you!

@superyyrrzz
Copy link
Contributor

@icnocop Thank you for the updates! I will review soon.

PDF [command] to combine them

This just means use the HtmlToPdfConverter to generate PDF, as what your PR already did. Run docfx.exe against a config with build and pdf section is equivalent to run docfx.exe build and docfx.exe pdf. The latter is what I mean by "pdf command".

Added {{{conceptual}}} to cover HTML template
Removed unneeded cover page processor
@icnocop
Copy link
Contributor Author

icnocop commented Apr 11, 2019

Thanks again for your guidance! 💯

I've pushed another commit with the changes as you've suggested.

…ontents"

Using FilePathComparer.OSPlatformSensitiveRelativePathComparer to compare directory names
Require cover.md to be in the same directory as toc.yml
@superyyrrzz superyyrrzz merged commit f1ff9c2 into dotnet:dev Apr 12, 2019
@superyyrrzz
Copy link
Contributor

@icnocop Thank you for contribution for this amazing feature!

@icnocop
Copy link
Contributor Author

icnocop commented Apr 12, 2019

You're welcome. Thank you @superyyrrzz for the quick feedback! 🎉

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