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

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

Merged
merged 2 commits into from
Nov 7, 2018

Conversation

moltenform
Copy link
Contributor

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.

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()
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@laurent22
Copy link
Owner

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;');
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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>');
Copy link
Owner

@laurent22 laurent22 Nov 2, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Revising based on feedback
Also, a couple changes to tests so that they pass in Windows.
@laurent22
Copy link
Owner

Looks good now, thanks @downpoured!

@laurent22 laurent22 merged commit 251f1bb into laurent22:master Nov 7, 2018
@moltenform moltenform deleted the titlesForExportedPdf branch November 8, 2018 18:13
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.

2 participants