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

Escape roundtrip #366

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

threefjefff
Copy link
Contributor

@threefjefff threefjefff commented Jan 16, 2025

Closes [#234]

Stops escaped character sequences being merged back together with other text sequences.
Registers and handles serializtion/deserialization of escape mark type.

Treat the start of inlining a parent block the same way as the begining of a line (to catch sequences where, for example, we've fought with the editor in rich text mode to allow us to insert a # without it turning into a header, thereby serialising to - \#)

PR Checklist

  • All new/changed functionality includes unit and (optionally) e2e tests as appropriate
  • All new/changed functions have /** ... */ docs
  • I've added the bug/enhancement and other labels as appropriate

Environment(s) tested

  • Device: [e.g. desktop, iPhone6, etc.]
  • OS: [e.g. iOS]
  • Browser [e.g. chrome, safari]
  • Version [e.g. 22]

Additional context

Add any other context about the PR here.

Copy link
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

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

Thanks @threefjefff for the PR.
Overall I think the PR is going in the right direction. 🎉

You should add a unit test for the preserve-escape rule and see if it is worth adding a regression test for the issue Dion reported. Perhaps in the markdown-serializer unit test file.

Great work so far! You can see how much code you ended up writing to fix a relatively simple bug - this is why we should be intentional going forward about what is worth fixing and what is not. More code = more maintenance, more cognitive overhead, more bugs potential, etc...

For this specific bug I think we can go ahead and merge once the PR is in a good state with its relevant tests. Thanks for now. 🙂


escape: {
parseDOM: [
{ style: "span.escaped" }
Copy link
Contributor

Choose a reason for hiding this comment

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

would probably use a TagParserRule here for consistency with the rest of the rules we define
parseDOM: [{ tag: "span.escaped" }],

Copy link
Contributor

@dancormier dancormier left a comment

Choose a reason for hiding this comment

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

Thanks for the bug fix @threefjefff. I ran npm run format to resolve some of the formatting inconsistencies and get the lint job passing (8c7a3e0). Other than that, the only things I think necessary to get this merged is what Giamir mentioned.

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.

3 participants