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

Export note with local images #1306

Merged
merged 7 commits into from
Feb 10, 2018
Merged

Conversation

nlopin
Copy link
Contributor

@nlopin nlopin commented Dec 17, 2017

Looks through the note and searches for local images. Copies them to ‘images’ folder in the export path and replaces affected links.

#1261

Looks through the note and searches for local images. Copies them to ‘images’ folder in the export path and replaces affected links.

BoostIO#1261
@BoostnoteBot
Copy link
Collaborator

Be sure to be changed browser/main/Detail/SnippetNoteDetail.js.

@kazup01 kazup01 added the awaiting review ❇️ Pull request is awaiting a review. label Dec 18, 2017
@kazup01 kazup01 requested review from sota1235 and removed request for sota1235 December 20, 2017 08:54
@nlopin
Copy link
Contributor Author

nlopin commented Dec 27, 2017

@kazup01 any news?

@kazup01
Copy link
Member

kazup01 commented Dec 28, 2017

@nlopin Thanks for your support! We will review it at sometime soon.
Thank you for waiting :)

@kazup01 kazup01 added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Jan 4, 2018
@kazup01
Copy link
Member

kazup01 commented Jan 4, 2018

Sorry for the long absence. Could you fix some conflicts?

@nlopin
Copy link
Contributor Author

nlopin commented Jan 5, 2018

Sure

@nlopin
Copy link
Contributor Author

nlopin commented Jan 6, 2018

@kazup01 it will be an issue with html export (#1256). There is a code to receive an HTML for export:

this.exportAsDocument('html', (value) => {
      return this.refs.root.contentWindow.document.documentElement.outerHTML
})

As you can see, the whole HTML of the preview window is fetched and exported then. But, it contains relative links to local styles and fonts. For example, all of those styles will be lost after user copy the exported HTML to another computer:

<link rel="stylesheet" href="file:///Users/nlopin/Workspace/Boostnote/node_modules/katex/dist/katex.min.css">
<link rel="stylesheet" href="file:///Users/nlopin/Workspace/Boostnote/node_modules/codemirror/lib/codemirror.css">
<link rel="stylesheet" id="codeTheme" href="file:///Users/nlopin/Workspace/Boostnote/node_modules/codemirror/theme/dracula.css">

It is not a good idea to export content like this because it does not provide consistent view on different machines.

We do have markdown-it plugin and I want to try it for rendering an HTML output. But we need to decide what to do with our custom styles:

  1. Should we keep them and include to the export, or ignore them?
  2. Custom font "Lato". It's impossible to embed it into the page. Any ideas what can we do here?

@Rokt33r
Copy link
Member

Rokt33r commented Jan 13, 2018

@nlopin

  1. I think we can insert style into <head> element of html file.
  2. We can't. The user must install their custom font to each machine.
    (I think we should provide a proper instruction, how to install the default font, Lato.)

@nlopin
Copy link
Contributor Author

nlopin commented Jan 13, 2018

@Rokt33r thanks for your response.

Looks like you didn't catch the issue in the first question. The question is not about where should we put styles. The issue is: We'll have to export those css files along with html. Otherwise it won't work properly.

If it's ok I'll do this.

@Rokt33r
Copy link
Member

Rokt33r commented Jan 14, 2018

@nlopin It makes sense. I've realized now that it is just like what Chrome does.

Yea, we should export multiple files, style sheet, images and any other static stuff, including fonts if url is available.

@Rokt33r
Copy link
Member

Rokt33r commented Jan 25, 2018

@nlopin ping.

1 similar comment
@Rokt33r
Copy link
Member

Rokt33r commented Feb 3, 2018

@nlopin ping.

@nlopin
Copy link
Contributor Author

nlopin commented Feb 3, 2018

@Rokt33r hi. Sorry for delay. I'm working on it right now.

@nlopin
Copy link
Contributor Author

nlopin commented Feb 5, 2018

@Rokt33r @kazup01 I've finished.

Export is divided by two parts:

  1. export additional files and paths modifications
  2. export the content of the note

Additional files are exported via copy tasks. Task is an object {'src': pathfrom, 'dst': pathto}. Destination path can be relative or absolute.

If all additional files are exported, the content will be exported at the end. If an error happened during export, boostnote remove all already exported files and folders.

Feel free to ask questions here or at Slack.

@Rokt33r
Copy link
Member

Rokt33r commented Feb 5, 2018

@nlopin Great work! 👍 I'll review this tomorrow!

@Rokt33r Rokt33r requested review from Rokt33r and removed request for kohei-takata February 5, 2018 10:36
@Rokt33r Rokt33r added awaiting review ❇️ Pull request is awaiting a review. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Feb 5, 2018
@tatoosh
Copy link

tatoosh commented Feb 5, 2018

Will u also include CSS ?

@nlopin
Copy link
Contributor Author

nlopin commented Feb 5, 2018

@tatoosh yes. If you export to HTML two additional folders will be exported – images and css

@tatoosh
Copy link

tatoosh commented Feb 5, 2018

THX @nlopin!
Do you have an option for Export CSS, so Form dark theme the Export will transformed to white?

@nlopin
Copy link
Contributor Author

nlopin commented Feb 6, 2018

No, it will export the same styles you see on preview. Create feature request if you need it

function saveToFile (data, filename) {
return new Promise((resolve, reject) => {
fs.writeFile(filename, data, (err) => {
if (err) throw err
Copy link
Member

Choose a reason for hiding this comment

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

I assume this makes the error go limbo. return reject(err) should be right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@nlopin
Copy link
Contributor Author

nlopin commented Feb 6, 2018

@Rokt33r done! Thank you

@Rokt33r
Copy link
Member

Rokt33r commented Feb 7, 2018

Now we need to implement UI. I'm going to create another PR against this branch about tomorrow. Let me know if you want to finish it by yourself!

@nlopin
Copy link
Contributor Author

nlopin commented Feb 7, 2018

Could you explain what do you mean?
There is no sense in current version of HTML export, because it doesn't work properly. I think it's ok to merge PR because it fixes that issue.

@nlopin
Copy link
Contributor Author

nlopin commented Feb 9, 2018

@kazup01 @Rokt33r I'm worried that leaving this PR without merge will lead us to new set of conflicts.

@Rokt33r
Copy link
Member

Rokt33r commented Feb 10, 2018

@nlopin sorry for my late answer. Everything looks great. I'll merge this today!

@Rokt33r
Copy link
Member

Rokt33r commented Feb 10, 2018

I found a problem.

screen shot 2018-02-10 at 9 11 57 am

Styling of task list seems to be broken after exporting as html. I'm investigating now.

@Rokt33r Rokt33r merged commit 0476ff7 into BoostIO:master Feb 10, 2018
@Rokt33r
Copy link
Member

Rokt33r commented Feb 10, 2018

Merged!

@Rokt33r Rokt33r added next release (v0.9.0) and removed awaiting review ❇️ Pull request is awaiting a review. labels Feb 10, 2018
@nlopin
Copy link
Contributor Author

nlopin commented Feb 10, 2018

This is the fundamental issue. Some styles applies directly to Preview HTML in rewriteIframe method. It's not a good practice, so I removed that.

I add class to the li tag at markdown to html transformation and then style it with css. @Rokt33r Please, have a look

@Rokt33r
Copy link
Member

Rokt33r commented Feb 10, 2018

@nlopin Could you create another PR? I can review it now!

@nlopin
Copy link
Contributor Author

nlopin commented Feb 10, 2018

Done #1531

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

5 participants