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

Show note title in pdf export (#890) #937

Merged
merged 2 commits into from Nov 7, 2018

Conversation

Projects
None yet
2 participants
@moltenjs
Contributor

moltenjs commented Oct 31, 2018

An example of a possible approach, to temporarily change the html in the webview to show the note title. Works, but there may be a more elegant fix.

Ben Fisher
Show note title in pdf export (#890)
An example of a possible approach, to temporarily change the html in the webview to show the note title. Works, but there may be a more elegant fix.
// refresh the webview contents, undoing our temporary change
this.lastSetHtml_ = '';
this.render()

This comment has been minimized.

@laurent22

laurent22 Nov 2, 2018

Owner

A bit hacky but indeed probably the best way to refresh the content. Could we use this.forceUpdate() though instead of calling render directly?

This comment has been minimized.

@moltenjs

moltenjs Nov 4, 2018

Contributor

Done.

@laurent22

This comment has been minimized.

Owner

laurent22 commented Nov 2, 2018

Thanks, your approach is the right way as the app is going to print to PDF whatever is displayed in the HTML view so the HTML code indeed needs to be modified that way.

const tag = 'h2';
let splitStyle = s.split('</style>');
const index = splitStyle.length > 1 ? 1 : 0;
let toInsert = heading.replace(/&/g,'&amp;').replace(/</g,'&lt;').replace(/>/g,'&gt;');

This comment has been minimized.

@laurent22

laurent22 Nov 2, 2018

Owner

I was sure there was already a function to escape HTML somewhere but looks like there isn't. Any chance you could add an escapeHtml function to string-utils and use it here? Something like this for instance https://stackoverflow.com/a/4835406/561309

This comment has been minimized.

@moltenjs

moltenjs Nov 4, 2018

Contributor

I had looked, and the only one was buried within React and not exposed. Done.

insertHtmlHeading_(s, heading) {
const tag = 'h2';
let splitStyle = s.split('</style>');

This comment has been minimized.

@laurent22

laurent22 Nov 2, 2018

Owner

Hmm... what if later on a style block is added at the bottom of a note - wouldn't that mean that the title would end up there too? Is it not possible to insert the title based on the tag? Or maybe a hidden placeholder could be added to the generated HTML for example as a comment - <!-- TITLE_PLACEHOLDER --> then this placeholder would be replaced here.

This comment has been minimized.

@moltenjs

moltenjs Nov 5, 2018

Contributor

Not sure what you mean by "insert the title based on the tag"? the code as written inserts the heading after the first </style>, so it would still work as expected if there are other </style> blocks.

I'm now using a comment placeholder now which is less fragile in the long term.

Ben Fisher
Show title in pdf export
Revising based on feedback
Also, a couple changes to tests so that they pass in Windows.
@laurent22

This comment has been minimized.

Owner

laurent22 commented Nov 7, 2018

Looks good now, thanks @downpoured!

@laurent22 laurent22 merged commit 251f1bb into laurent22:master Nov 7, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@moltenjs moltenjs deleted the moltenjs:titlesForExportedPdf branch Nov 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment