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

Html to md feature #3099

Merged
merged 13 commits into from
Sep 2, 2019
Merged

Html to md feature #3099

merged 13 commits into from
Sep 2, 2019

Conversation

AWolf81
Copy link
Contributor

@AWolf81 AWolf81 commented Jun 29, 2019

Description

I'm addressing the requested review from PR #1980. If I've missed anything please let me know. @StormBurpee I've based this on your branch and addressed the review comments - hope that's OK.

I think the feature is working as expected. The only thing that I have noticed is that the image is not copied to the created note.

What do you think can we address that after this PR or should I put some work into it and check it? But I think we would have to discuss this a bit e.g. copy the image as an asset or create a link to the original location. I think copying would be better.

To the test:
Is the test OK? It just checks that the created note is written to disc and that the keys are correct. I think that's OK as we don't have to check that Turndown is converting the html to markdown correctly.
Or is there anything else to test?

Issue fixed

#923

Type of changes

  • ⚪ Bug fix (Change that fixed an issue)
  • ⚪ Breaking change (Change that can cause existing functionality to change)
  • ⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
  • 🔘 Feature (Change that adds new functionality)
  • ⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • 🔘 My code follows the project code style
  • 🔘 I have written test for my code and it has been tested
  • 🔘 All existing tests have been passed
  • ⚪ I have attached a screenshot/video to visualize my change if possible

Screenshots

Link below the other new note options
grafik

Import dialog with error text
grafik


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Jul 1, 2019
Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

Can you remove the package-lock.json file please? Thank you 👍

reject({result: false, error: 'Please check your URL is in correct format. (Example, https://www.google.com)'})
}

const request = url.includes('https') ? https : http
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace this with url.startsWith("https") instead?

@ZeroX-DG ZeroX-DG 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 Jul 7, 2019
@ZeroX-DG
Copy link
Member

@AWolf81 Can you fix the wrong color for dracula theme?
image

Also, it seem that the code doesn't work on tables, can you check again? For example, try this URL and take a look at the table section.
https://github.com/adam-p/markdown-here/wiki/Markdown-Cheatsheet

@AWolf81
Copy link
Contributor Author

AWolf81 commented Jul 28, 2019

@ZeroX-DG fixed Dracula theme and added the gfm turndown plugin.

Table generation is not working perfectly but I'm not sure how to fix it. Text between two tables is not correctly rendered (see below screenshot). Adding a new line after the first table fixes the rendering. With-out the space it is rendering the three parts in tbodys. With the space the text is a p-tag and the rest are two tables.
I'll create a minimal example to reproduce the issue and maybe I ask at turndown repo. why it is occuring.

grafik

OK, the issue is not Turndown. It's caused by markdown-it-multimd-table plugin because the markdown created by Turndown is correct.

Please have a look at the following minimal example:
https://codesandbox.io/s/turndown-convert-html-to-md-render-with-markdownit-93cqt

Update

OK, the above issue with the "p-tag" with-in the table can't be avoided as MultiMd Plugin is handling every new line that contains a | as a new tbody (see referenced issue below). So the only workaround is to add a newline before the paragraph.

So we can't improve that behavior.

@Flexo013 Flexo013 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 Jul 28, 2019
const html = document.createElement('html')
html.innerHTML = data

const scripts = html.getElementsByTagName('script')
Copy link
Member

Choose a reason for hiding this comment

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

@AWolf81 Instead of doing this, can we use the turndown remove rule?
https://github.com/domchristie/turndown#removefilter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ZeroX-DG good idea. I'll change this in one week as I'm without a computer at the moment.

remove filter should simplify this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ZeroX-DG OK, I've changed the filtering. I've created a basic Codesandbox example to check that the script tag is not available in Bootstnote.

https://4bckd.csb.app/

If you open the URL in the browser it will execute an alert and in BoostNote there is no script tag available.
Is there more I can test?

The html constant can be removed as Turndown is handling the conversion string to md - it was just there for the script filtering.

Copy link
Member

Choose a reason for hiding this comment

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

I think it looks good now. Thank you for your contribution!

@ZeroX-DG ZeroX-DG 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 Aug 26, 2019
Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@ZeroX-DG ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Aug 31, 2019
@Rokt33r Rokt33r removed the approved 👍 Pull request has been approved by sufficient reviewers. label Sep 2, 2019
@Rokt33r Rokt33r added this to the v0.13.0 milestone Sep 2, 2019
@Rokt33r Rokt33r merged commit 68b3077 into BoostIO:master Sep 2, 2019
@AWolf81 AWolf81 deleted the html-to-md branch September 2, 2019 22:06
@Flexo013
Copy link
Contributor

Flexo013 commented Oct 23, 2019

Found an edgecase where this can crash the new note menu: #3308

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.

5 participants