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

Desktop: Fixes #8448: Merge changes from upstream turndown project #8468

Conversation

personalizedrefrigerator
Copy link
Collaborator

@personalizedrefrigerator personalizedrefrigerator commented Jul 13, 2023

Summary

The upstream turndown project has bug fixes related to code blocks and whitespace. This should fix #8448 and should partially fix #8435.

Notes

Because this pull request may require further testing, it's still a draft.

Comment on lines +219 to +222
const replaceNonbreakingSpaces = space => {
// \u{00A0} is a nonbreaking space
return space.replace(/\u{00A0}/ug, this.options.nonbreakingSpace);
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this function after merging changes to optionally replace \u00A0 characters with  , making non-breaking spaces noticeable within the markdown editor.

@@ -5,5 +5,6 @@
| [Source](https://github.com/nim-lang/nim) | The github project |
| [nimble](https://github.com/nim-lang/nimble) | The nim package manager |
| [choosenim](https://github.com/dom96/choosenim) | Toolchain installer |
| | |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The HTML input file corresponding to this test contains

		<tr>
			<td>&nbsp;</td>
			<td></td>
		</tr>

which was previously ignored.

It is no longer ignored and thus the test output has been updated.

@laurent22
Copy link
Owner

Thanks, I think that's a good change, as long as our existing test units are still passing.

  • Could you also update the README to mention on which version our fork will now be based?
  • Are you able to check that we aren't overwriting our custom code? (that was added to support the web clipper)

There's also turndown-plugin-gfm that contains a lot of improvements for the web clipper, but if it needs to be updated we can do so in a separate PR.

@personalizedrefrigerator
Copy link
Collaborator Author

personalizedrefrigerator commented Jul 14, 2023

I've done a small amount of testing and WebClipper still works (and produces similar output).

I'm testing with WebClipper from the Chrome store and the development version of Joplin (changing the web clipper port accordingly).

Example

Clipping the main GitHub repository page for Joplin

Old version of turndown:
turndown-test-old--markdown.md
turndown-test-old--rendered.pdf

New version:
turndown-test-old--markdown.md
turndown-test-new--rendered.pdf.pdf
(Edit: Fixed new version PDF possibly rendered from old markdown)

I've updated the turndown README with information about the commit that changes are based on.

@personalizedrefrigerator personalizedrefrigerator marked this pull request as ready for review July 14, 2023 20:52
@laurent22
Copy link
Owner

Looks good, thanks @personalizedrefrigerator!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants