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

Multiple fixes & enhancements #56

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

Conversation

dimrozakis
Copy link
Contributor

Hi there @comwes.

First off, thanks for creating this plugin, I've been using it to build docs for more than a year and it's been really useful!

I just gathered some old & new fixes and improvements I've made, rebased them, and submitted them here to be included upstream.

Please check the commit log and review each commit separately. If you only want to merge some of these changes, let me know and I'll rebase the unwanted commits out of ths PR.

Thanks!

trogper and others added 9 commits April 2, 2022 16:56
This reintroduces commit d26b56e

That commit was previously merged to master with PR comwes#21 to fix issue comwes#4.
But subsequently, MR comwes#26 undid the change. As far as I can tell this
happened by mistake during merge/rebase and conflict resolutions.
1. Remove duplicate declaration of `get_path_to_pdf`.
2. Properly handle `output_path` with no dir part (eg `docs.pdf`).
   This properly resolves issue comwes#27 that wasn't actually fixed with
   5d30cdd.
3. Fix relative link to PDF
   It had on `..` too many. This did't manifest as a problem when a site
   was deployed directly on a hostname but when deployed under a
   subdirectory, the PDF link wouldn't resolve.
Very simple styling, blockquotes appear in lighter color, indented, and
with a bar running down its left side.
If one can see the bookmarks index in the PDF viewer, seeing all of the
TOC sections there just clutters the index.
So that they do not appear in page headings.
For example the page edit link (pencil icon) and any other links added
by material theme on the right of the page header, inside the article
content.

Without this change, there'd be no icon but the entire URL for editing
the page would be included in the PDF above each page title which was
undesired.

Filtering of such links is based on the `md-content__button` class as
suggested in squidfunk/mkdocs-material#1920
All sections and articles would start with a <h1> which would mess up
the autogenerated bookmarks index displayed by the PDF viewer.

Initially I experimented with increasing the heading levels of a page
based on how deeply nested under subsections the page is.
This had some issues:

1. If a page was nested under N sections/dirs and used headings up to
   level L where N + L > 6, the headings' level couldn't be increased
   any more. I resorted to transforming them to `<strong>`.
1. Pages were now themed differently. The title of a page would be an
   `<h1>` if on top level, `<h2>` if under a top level section, `<h3>`
   if under a subsection and so on.
1. The top page banner that indicates the current document would just
   display the top level section for non top-level pages.

In the end I decided that it's best to not mess with the heading levels
of pages, just with their nesting in the PDF's bookmark index.

This was achieved via the use of the `bookmark-level` CSS property.

Therefore this commit shouldn't affect the contents of the output PDF at
all, just the nesting of the pages & sections in its bookmark index.
@vladdoster
Copy link

Don't forget to squash your commits ;).

@dimrozakis
Copy link
Contributor Author

Don't forget to squash your commits ;).

Hi @vladdoster. I submitted this PR with multiple commits because each one does a single, mostly independent thing and has a descriptive commit message. This should make review easier (by reviewing each smaller commit separately, as suggested in the PR description). Also, if only some but not all changes are desired, this makes it easier to exclude those commits that correspond to particular undesired changes during a later rebase.

Did you perhaps have a chance to review and/or test my suggested changes or have any other kind of feedback on them?

@dimrozakis
Copy link
Contributor Author

Hi @comwes.

Have you had a chance to take a look at this MR? Do you have any feedback? Are you interested in merging any of the included changes?

@vladdoster
Copy link

@dimrozakis My comment was in jest as I was having trouble using mkpdfs-mkdocs and came across this issue.

That said, this PR fixed my issues, and can vouch that it works.

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.

3 participants