Markdown: enable removing or keeping empty paragraphs of an HTML value
#3725
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds a new removeEmptyParagraphs option and Unified plugin to strip empty nodes from rendered Markdown, exposes it as a reflected Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Component as limel-markdown (component)
participant Watcher as Watcher
participant Parser as markdownToHTML
participant Sanitize as Sanitize HTML
participant RemoveEmpty as RemoveEmptyParagraphs Plugin
participant LazyLoad as LazyLoadImages Plugin
participant DOM as Rendered HTML
Component->>Parser: render(value, { removeEmptyParagraphs, lazyLoadImages, ... })
Note over Parser,Sanitize: Processing pipeline
Parser->>Sanitize: sanitize(html)
Sanitize-->>RemoveEmpty: sanitized AST
RemoveEmpty-->>LazyLoad: transformed AST
LazyLoad-->>DOM: transformed HTML
DOM-->>Component: rendered output
Component->>Watcher: removeEmptyParagraphs attribute changed
Watcher-->>Component: textChanged() -> re-render -> Parser
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
changed the title
@coderabbitaiEnable property reflection for lazyLoadImages
Kiarokh
changed the title
value
Kiarokh
force-pushed
the
markdown-empty-paragraphs
branch
from
251fd74 to
2748154
Compare
Contributor
Author
|
@coderabbitai review plz |
| * whitespace (`<br />` or ` `). | ||
| */ | ||
| @Prop({ reflect: true }) | ||
| public removeEmptyParagraphs = true; |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Contributor
Author
There was a problem hiding this comment.
Oops… sorry the title was wrong. This is a feature for limel-markdown not limel-text-editor. Some 🧠 💨
⚡ ed the title and the commit now.
However, I do agree to your points. I was actually just writing in Slack to see if this approach is good. We would preferably need to remove the empty paragraphs, when the email is being imported, and not leave it up to this innocent component.
But this PR can help make my point and offer a way to sanitize the value before it's saved. I'll add more on Slack
Contributor
There was a problem hiding this comment.
Oh, hang on, was this limel-markdown? Why did I think it was limel-text-editor? 🤨
Anyway, forget what I said above. I think this property is fine in limel-markdown 👍
Contributor
There was a problem hiding this comment.
Ah! So it wasn't my mistake then! 😅
Contributor
There was a problem hiding this comment.
However, I do agree to your points. I was actually just writing in Slack to see if this approach is good. We would preferably need to remove the empty paragraphs, when the email is being imported, and not leave it up to this innocent component.
But this PR can help make my point and offer a way to sanitize the
valuebefore it's saved. I'll add more on Slack
Yeah, I agree. My original comments still apply in this case, but it's less of a problem for limel-markdown than it would have been in limel-text-editor, hence why I think it's acceptable to add this prop to limel-markdown 👍
Contributor
Author
There was a problem hiding this comment.
sorry, yes it was misleading info by me first.
this is a tricky scenario since emails are imported in CMR, and then might get compiled again from the CRM and be sent out. So we may not want to modify their content too much, as they are rendered differently in the Email Clients, than in CRM, and may appear malformatted when they reach the reader. But we still want to render them decently in the CRM, without re-creating the stupid "no-space-between-paragraphs" visual style of Eail Clients everywhere.
Contributor
There was a problem hiding this comment.
Some preprocessing can perhaps be done by a limebb- component before passing the value to limel-markdown for rendering. Or the database could have two columns, one where we store the original email, and one where we store a sanitised version for display in the app.
🧩 Analysis chain@Kiarokh Thanks! I’ve pulled the branch and did a focused pass. Here’s the review summary and concrete fixes: Key findings
Suggested patches
// src/components/markdown/markdown.tsx
/**
* Enable lazy loading for images
*/
- @Prop()
- @Prop({ reflect: true })
+ @Prop({ reflect: true })
public lazyLoadImages = false;
// src/components/markdown/remove-empty-paragraphs-plugin.ts
const NBSP_REGEX = /\u00A0/g;
+const ZERO_WIDTH_REGEX = /[\u200B\u200C\u200D\uFEFF]/g;
const isWhitespace = (value: string): boolean => {
if (!value) {
return true;
}
- return value.replaceAll(NBSP_REGEX, ' ').trim() === '';
+ // Normalize NBSP and zero-width chars to regular spaces, then trim.
+ const normalized = value.replaceAll(NBSP_REGEX, ' ')
+ .replaceAll(ZERO_WIDTH_REGEX, '');
+ return normalized.trim() === '';
};
// src/components/markdown/remove-empty-paragraphs-plugin.spec.ts
it('removes paragraphs with only a <br> (with and without class)', () => {
const tree = createRoot([
createParagraph([createElement('br')]),
createParagraph([createElement('span', [createElement('br')])]),
createParagraph([createElement('br', [], { className: 'foo' })]),
]);
runPlugin(tree, true);
expect(tree.children).toHaveLength(0);
});
it('removes paragraphs that contain only zero-width whitespace', () => {
const zwsp = '\u200B\u200C\u200D\uFEFF';
const tree = createRoot([
createParagraph([createText(zwsp)]),
createParagraph([createElement('span', [createText(zwsp)])]),
]);
runPlugin(tree, true);
expect(tree.children).toHaveLength(0);
});
/* src/components/markdown/examples/markdown-remove-empty-paragraphs.scss */
:host(limel-example-markdown-remove-empty-paragraphs) {
display: grid;
- gap: 1 rem;
- grid-template-columns: repeat(auto-fit, minmax(10 rem, 1 fr));
+ gap: 1rem;
+ grid-template-columns: repeat(auto-fit, minmax(10rem, 1fr));
}
limel-markdown {
display: block;
- padding: 0.5 rem 1 rem;
+ padding: 0.5rem 1rem;
}Docs
Questions
If you apply the above, I’m happy to give a quick re-check. 🏁 Script executed: Length of output: 1668 @Kiarokh Thanks for the details and for re-running. I’ve pulled fuller contexts this time. Here’s a crisp review with concrete next steps: What I verified
Recommended changes
// src/components/markdown/remove-empty-paragraphs-plugin.ts
const NBSP_REGEX = /\u00A0/g;
+const ZERO_WIDTH_REGEX = /[\u200B\u200C\u200D\uFEFF]/g;
function isWhitespace(value: string): boolean {
if (!value) {
return true;
}
- return value.replaceAll(NBSP_REGEX, ' ').trim() === '';
+ const normalized = value.replaceAll(NBSP_REGEX, ' ')
+ .replaceAll(ZERO_WIDTH_REGEX, '');
+ return normalized.trim() === '';
}
// src/components/markdown/remove-empty-paragraphs-plugin.spec.ts
it('removes paragraphs with only a <br>', () => {
const tree = createRoot([
createParagraph([createElement('br')]),
createParagraph([createElement('span', [createElement('br')])]),
]);
runPlugin(tree, true);
expect(tree.children).toHaveLength(0);
});
it('removes paragraphs that contain only zero-width whitespace', () => {
const zw = '\u200B\u200C\u200D\uFEFF';
const tree = createRoot([
createParagraph([createText(zw)]),
createParagraph([createElement('span', [createText(zw)])]),
]);
runPlugin(tree, true);
expect(tree.children).toHaveLength(0);
});
/* src/components/markdown/examples/markdown-remove-empty-paragraphs.scss */
:host(limel-example-markdown-remove-empty-paragraphs) {
display: grid;
- gap: 1 rem;
- grid-template-columns: repeat(auto-fit, minmax(10 rem, 1 fr));
+ gap: 1rem;
+ grid-template-columns: repeat(auto-fit, minmax(10rem, 1fr));
}
limel-markdown {
display: block;
- padding: 0.5 rem 1 rem;
+ padding: 0.5rem 1rem;
}Optional considerations
If you apply the above tweaks, I’m happy to re-check quickly. 🏁 Script executed: Length of output: 24019 @Kiarokh Thanks for the rerun—here’s a focused follow-up based on the actual files. What I verified
Recommended changes
// src/components/markdown/markdown.tsx
@Watch('lazyLoadImages')
public handleLazyLoadImagesChange() {
return this.textChanged();
}
@Watch('whitelist')
public handleWhitelistChange() {
return this.textChanged();
}
// src/components/markdown/remove-empty-paragraphs-plugin.ts
const NBSP_REGEX = /\u00A0/g;
const ZERO_WIDTH_REGEX = /[\u200B\u200C\u200D\uFEFF]/g;
const isWhitespace = (value: string): boolean => {
if (!value) {
return true;
}
const normalized = value.replaceAll(NBSP_REGEX, ' ')
.replaceAll(ZERO_WIDTH_REGEX, '');
return normalized.trim() === '';
};
// src/components/markdown/remove-empty-paragraphs-plugin.spec.ts
it('removes paragraphs with only a <br> (with or without wrapper span)', () => {
const tree = createRoot([
createParagraph([createElement('br')]),
createParagraph([createElement('span', [createElement('br')])]),
]);
runPlugin(tree, true);
expect(tree.children).toHaveLength(0);
});
it('removes paragraphs that contain only zero-width whitespace', () => {
const zw = '\u200B\u200C\u200D\uFEFF';
const tree = createRoot([
createParagraph([createText(zw)]),
createParagraph([createElement('span', [createText(zw)])]),
]);
runPlugin(tree, true);
expect(tree.children).toHaveLength(0);
});
// Optional: nested <p> inside <p>
it('removes a paragraph that only contains an empty nested paragraph', () => {
const tree = createRoot([
createParagraph([createParagraph()]),
]);
runPlugin(tree, true);
expect(tree.children).toHaveLength(0);
});Notes
Want me to open a follow-up test/cleanup PR if you prefer to keep this one lean? ✅ Actions performedReview triggered.
|
Kiarokh
changed the title
valuevalue
To enable consumer to keep or remove empty paragraphs from their HTML `value` input, before rendering the content.
Kiarokh
force-pushed
the
markdown-empty-paragraphs
branch
from
2748154 to
6e456f7
Compare
Contributor
Author
|
@coderabbitai what do you think about the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5626 @Kiarokh Excellent fixup! 👍 The fixup commit looks great—you've addressed the two main technical gaps I flagged: ✅ Zero-width whitespace handling
✅ Test coverage
✅ Example enhancement
Approach: Using a Remaining optional item from my earlier review:
The core plugin implementation and tests are now solid. Nice work! 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When an email is imported into the CRM, the spacing between paragraphs in the CRM appears to be much larger than the spacing between paragraphs in emails, created in Outlook. This is visible in the Activity Feed, and creates a lot of additional vertical scrolling for users.
This issue stems from how email clients like Outlook handle paragraphs. Unlike standard text editing programs like Word, Outlook (or Gmail) doesn’t follow the standard typographic rules. In Word, there’s usually a greater space between paragraphs than between lines. However, users often hit the Enter key twice to create a gap between lines in while they are typing their emails.
When these emails are imported into the CRM, our standard paragraph styling adds even more space, creating the appearance of double line spacing because there are extra empty paragraphs in the actual imported email.
We don’t want to have a non-standard typographic style in CRM. So we can't simply remove our default styling for margins between paragraphs.
So we need to remove the empty paragraphs from the input
valuethat hasHTMLformat. The issue is that the depending on the email client which is used, the empty paragraphs would be different. They could be anything like:Therefore, we can’t do this purely with CSS in a fully reliable way, since CSS selectors can’t inspect text content. So something like:
… still won’t detect invisible text like
or empty nested paragraphs inside each other.Additionally, this feel like something that should be controlled via a
propby the consumer. Because sometimes, they my really need the empty paragraphs for some legit reason.So we need JS to handle this, as done in this PR.
fix https://github.com/Lundalogik/limepkg-email/issues/1744
Summary by CodeRabbit
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: