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

Support Markdown import #2186

Merged
merged 19 commits into from
Aug 3, 2024
Merged

Conversation

finkef
Copy link
Collaborator

@finkef finkef commented Jul 24, 2024

Closes #867.

This PR adds an action for markdown import. It makes use of marked for parsing markdown into tokens and transform them into a supported plaintext format to be used with importText.

Tests are used to mainly cover the convertMarkdownToText function in favor of state mutations to decouple parsing markdown and handling potentially yet unsupported thoughts.

Open topics:

  • Determine how to trigger the action from the UI.
  • Support currently unsupported attributes, e.g. =blockquote, =code and =image.

Copy link
Contributor

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

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

Thanks, this looks very good. The token processor is well-documented and easy to read, and the test coverage looks great.

One thing is that separate lines need to be parsed as separate thoughts. There is no difference between a paragraph, newline, or list item in em. The only exception should be blockquotes and code blocks, where newlines are allowed, as we discussed, and will be supported in the future. Otherwise, they should be parsed as separate thoughts. I've pushed a couple simple tests to demonstrate. Another test should probably be added for nested multiple lines.

As for the UI, currently all types of supported content (mainly text and html) are imported via paste or by dragging and dropping a file into the editor. Both trigger importText. Ideally the user does not need a separate mechanism to import markdown. Can we autodetect markdown to determine whether their text needs to be passed through convertMarkdownToText?

src/actions/importMarkdown.ts Outdated Show resolved Hide resolved
src/actions/importMarkdown.ts Outdated Show resolved Hide resolved
src/actions/importMarkdown.ts Outdated Show resolved Hide resolved
src/actions/__tests__/importMarkdown.ts Outdated Show resolved Hide resolved
src/actions/__tests__/importMarkdown.ts Outdated Show resolved Hide resolved
src/actions/__tests__/importMarkdown.ts Outdated Show resolved Hide resolved
src/actions/importMarkdown.ts Outdated Show resolved Hide resolved
@finkef

This comment was marked as resolved.

@raineorshine

This comment was marked as resolved.

@finkef finkef requested a review from raineorshine July 26, 2024 08:33
src/util/__tests__/markdownToText.ts Outdated Show resolved Hide resolved
src/util/__tests__/markdownToText.ts Outdated Show resolved Hide resolved
src/util/__tests__/markdownToText.ts Outdated Show resolved Hide resolved
src/util/__tests__/markdownToText.ts Show resolved Hide resolved
src/util/__tests__/markdownToText.ts Outdated Show resolved Hide resolved
src/util/markdownToText.ts Show resolved Hide resolved
@finkef
Copy link
Collaborator Author

finkef commented Aug 2, 2024

Removed collapse pass and instead handling scope in initial pass. All tests passing with minimum ::/=scope application, pretty happy with this refactor.

Copy link
Contributor

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

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

Looking good!

I did find one case that's not working as expected. There is a ::/=scope false positive. I pushed a test in 218a63b.

src/util/__tests__/markdownToText.ts Outdated Show resolved Hide resolved
src/util/__tests__/markdownToText.ts Outdated Show resolved Hide resolved
src/util/__tests__/markdownToText.ts Show resolved Hide resolved
src/util/markdownToText.ts Outdated Show resolved Hide resolved
src/util/markdownToText.ts Outdated Show resolved Hide resolved
src/util/markdownToText.ts Show resolved Hide resolved
src/util/markdownToText.ts Show resolved Hide resolved
src/util/markdownToText.ts Outdated Show resolved Hide resolved
@finkef
Copy link
Collaborator Author

finkef commented Aug 3, 2024

Fixed all comments and failing tests apart from the open TS target lib discussion.

src/util/__tests__/markdownToText.ts Outdated Show resolved Hide resolved
@raineorshine raineorshine merged commit db59f8b into cybersemics:main Aug 3, 2024
3 checks passed
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.

Import Markdown
2 participants