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(html-comment): fix html comments from being visible in the rich editor #211

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

giamir
Copy link
Contributor

@giamir giamir commented Aug 10, 2022

Closes #195

Hello, first time contributor here. 👋
I thought I would pick up an issue labeled as good first issue to familiarize myself with the codebase and your code conventions. I have to say the PR turned out to be a bit bigger than I initially thought. Nevertheless I hope this is useful and I look forward to your feedback.

Describe your changes

Issue Analysis
The blue bar in RT mode described in #195 appears because of this CSS rule which applies to all selected html_block nodes markdown-it parses out (html comments are treated as any another html_block node).
Furthermore I found that all the html comments get a margin bottom in RT mode (which I assume is not desired behavior) as a consequence of being categorized as any other html_block by the parser.

Proposed Solution
The solution I implemented introduces an html_comment markdown-it plugin which allows to differentiate html comments and handle them accordingly in the RT mode.
The plugin detects only html comments blocks.
Html comments inlined with other elements/text are ignored and they are processed as they were before.

<!-- a comment -->
✅ Detected
 
<!-- 
  a comment 
  over multiple lines 
-->
✅ Detected



some text <!-- a comment -->
❌ Ignored

<!-- a comment --> <p>some html</p>
❌ Ignored

A more detailed explanation about the plugin's logic can be found in the related unit test file (html-comment.test.ts).

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: Chrome
  • OS: MacOS 12.5
  • Browser: Chrome
  • Version: 99

@b-kelly b-kelly added bug Something isn't working mode - rich text Affects the editor's rich text (wysiwyg) mode html Issue with html parsing or rendering labels Aug 10, 2022
@b-kelly
Copy link
Collaborator

b-kelly commented Aug 10, 2022

Thank you for the PR! I'll try and review the next chance I get.

@giamir
Copy link
Contributor Author

giamir commented Aug 23, 2022

Hi @b-kelly,
Let me know if there is anything I can do to make your review process easier and prevent this branch from going stale. I tried to outline my solutioning in the PR summary but I'd also be open to go through the changes on a quick sync meeting if you prefer. Thanks.

@b-kelly
Copy link
Collaborator

b-kelly commented Aug 23, 2022

@giamir Sorry, I've been up to my eyeballs in non-Stacks-Editor related tasks this last week. I plan on getting a new release out by the end of the month (this week likely), so my goal is to have your PR reviewed and merged by then. Thank you for your patience!

@giamir
Copy link
Contributor Author

giamir commented Aug 23, 2022

No problem, @b-kelly.
I did not mean to create a sense of urgency.
I only wanted to offer support.
I appreciate your efforts as a maintainer and I can certainly empathise with busy times at work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working html Issue with html parsing or rendering mode - rich text Affects the editor's rich text (wysiwyg) mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(rich-text) Html comment at top translates into blue bar displayed across width of RT mode
2 participants