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

fix: better handling of adding the title to print and export to PDF #1744

Merged
merged 1 commit into from
Jul 29, 2019

Conversation

tessus
Copy link
Collaborator

@tessus tessus commented Jul 15, 2019

fixes #1743


When a document is printed or exported to PDF, a header 1 is prepended to the document with the title (internally adding a # Title).

This also adds an additional entry to the TOC with the title of the document. This PR fixes this behavior. I've also added a helper function to change the appearance in the future.

@tessus tessus added the desktop All desktop platforms label Jul 15, 2019
@tessus
Copy link
Collaborator Author

tessus commented Jul 15, 2019

@laurent22
Copy link
Owner

Hmm, I think we need a more permanent fix as this will break if the theme changes. In noteStyle.js, the styles for Markdown rendering are defined, in particular for h1. We could add to the list a new class like .print-header and apply the same styles - instead of h1 {... do h1, .print-header {... then create the header as <div class="print-header">...</div>. Not sure if that will work but it's better than having hard-coded style somewhere.

@tessus
Copy link
Collaborator Author

tessus commented Jul 15, 2019

Ok, I'm genuinely confused now. This makes no sense to me.

Printing should ignore the theme css anyway. Or do you want people to print black/blueish pages with grey/white text?
I have only used a similar css as the h1 is currently using, but this is not a header anyway, so we can use something that works for all themes (even though I still don't understand why). Printing will always be on a white or light background, so what's the point? I haven't used any colors (except for the line, which we can either remove or just remove the color). In that case, the text/line will be in the color of the main theme.
Either way, I'm still puzzled by your comment.

@tessus
Copy link
Collaborator Author

tessus commented Jul 20, 2019

@laurent22 did you see my comment? I really want to understand why supporting themes is needed for printing/exporting to PDF.
And I would like to get this in quickly. At least for me, it's rather annoying that I can't properly print/export a note with toc.

@laurent22
Copy link
Owner

Sorry I don't have time to look into this now, I'll check later.

@laurent22
Copy link
Owner

Printing should ignore the theme css anyway. Or do you want people to print black/blueish pages with grey/white text?

The style in noteStyle.js doesn't just define the colour but also padding, margin, font size, etc. I don't see why this particular element shouldn't have a CSS class like the other elements. Then that class can be easily styled in noteStyle.js, for example by making it an alias to h1

@tessus
Copy link
Collaborator Author

tessus commented Jul 23, 2019

I see what you mean. However, I actually wanted to change the size depending on the length of the title. I currently use 2em. h1 uses 1.5em. But IMO it should be bigger than h1. It's the title after all.
However, if there's a very long title, it would wrap over 2 or 3 lines, thus I would make it a bit smaller for longer titles. At leaat that was the plan for the future. I just wanted to fix it first.

Anyway, I'll try to use a css class. Let's see how it turns out.

@laurent22
Copy link
Owner

Ok let's merge for now and we can improve if later we notice that it's causing issues. Thanks for the update.

@laurent22 laurent22 merged commit ad8054b into laurent22:master Jul 29, 2019
@tessus
Copy link
Collaborator Author

tessus commented Jul 29, 2019

I will still work on a follow-up PR to implement a new css class. It's just my sis is in town, so it could take a bit longer.

@tessus tessus deleted the fix-title branch October 1, 2019 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

printing a document / exporting to PDF messes up the TOC
2 participants