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

Fix post text preview showing raw markdown #1431

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

hjiangsu
Copy link
Member

@hjiangsu hjiangsu commented Jun 5, 2024

Pull Request Description

This PR resolves an issue where the text preview for a post was showing raw markdown. To fix this, I used the markdown package to parse the markdown into it's AST tree, and took the textContent from the generated nodes. This seems to work for the most part, but may not fully render proper spacing for certain markdown.

Additionally, I made the text preview colour slightly lighter so that it doesn't clash with the main post title!

Issue Being Fixed

Issue Number: #1220

Screenshots / Recordings

Before After
image image
image image
image image

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

This looks good!

Keep in mind we already have a technique to convert markdown to plaintext by passing it through HTML. So maybe we could consolidate techniques.

final String htmlComment = markdownToHtml(commentContent);
final String plaintextComment = parse(parse(htmlComment).body?.text).documentElement?.text ?? commentContent;

Although in this case, the HTML is needed anyway for the rich text notification, so it doesn't hurt to perform both steps.

@hjiangsu
Copy link
Member Author

hjiangsu commented Jun 5, 2024

Ahh right - I completely forgot about that. I can go ahead and see if using the HTML parser works better!

@hjiangsu
Copy link
Member Author

hjiangsu commented Jun 5, 2024

I adjusted the code using the same technique as what you referenced earlier. It should be good now!

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

LGTM. The other technique parsed twice, and used body for the first pass and documentElement for the second. To be honest I don't remember why that was needed, but if this looks good in your testing, we'll go with it!

@hjiangsu
Copy link
Member Author

hjiangsu commented Jun 5, 2024

Ahh yeah weird... I didn't notice anything off when I tested it so it might be fine? I'll go ahead and merge this in and I'll monitor if anything looks off!

@hjiangsu hjiangsu merged commit 76b1fc1 into develop Jun 5, 2024
1 check passed
@hjiangsu hjiangsu deleted the fix/text-preview-markdown branch June 5, 2024 21:36
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.

2 participants