Skip to content

Fixed HTML rendered including empty trailing paragraphs#637

Open
ronaldlangeveld wants to merge 7 commits intobasecamp:mainfrom
ronaldlangeveld:fix-empty-paragraph
Open

Fixed HTML rendered including empty trailing paragraphs#637
ronaldlangeveld wants to merge 7 commits intobasecamp:mainfrom
ronaldlangeveld:fix-empty-paragraph

Conversation

@ronaldlangeveld
Copy link

@ronaldlangeveld ronaldlangeveld commented Jan 21, 2026

ref #441

  • Added normalizeEmptyContent helper function to convert Lexical's default <p><br></p> (used for empty editor content to maintain contentEditable functionality) into an empty string, preventing unwanted HTML from being submitted to the server.

  • Updated the value getter in src/elements/editor.js to apply this normalization after sanitizing the generated HTML, ensuring clean data persistence without breaking editor usability.

  • This fix addresses the issue where empty editors would submit <p><br></p> instead of an empty string, improving data integrity and user experience by avoiding meaningless HTML storage in Action Text fields.

  • Follows knowledge from my previous work on Ghost's Lexical Editor :)

ref basecamp#441

- Added `normalizeEmptyContent` helper function to convert Lexical's default `<p><br></p>` (used for empty editor content to maintain `contentEditable` functionality) into an empty string, preventing unwanted HTML from being submitted to the server.

- Updated the value `getter` in `src/elements/editor.js` to apply this normalization after sanitizing the generated HTML, ensuring clean data persistence without breaking editor usability.

- This fix addresses the issue where empty editors would submit `<p><br></p>` instead of an empty string, improving data integrity and user experience by avoiding meaningless HTML storage in Action Text fields.
Copy link
Collaborator

@samuelpecher samuelpecher 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 PR! I like the direct approach vs checking the node state.

I've left a few comments to just touch up the code in line with our conventions.

Comment on lines 175 to 177
this.cachedValue = normalizeEmptyContent(
sanitize($generateHtmlFromNodes(this.editor, null))
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the unnested route is clearer here

Suggested change
this.cachedValue = normalizeEmptyContent(
sanitize($generateHtmlFromNodes(this.editor, null))
)
const html = sanitize($generateHtmlFromNodes(this.editor, null))
this.cachedValue = normalizeEmptyContent(html)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This be rolled into HTML helpers file, or sanitization. We can keep it terse without the documentation, the intent is nice and clear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd lean towards dropping the unit test and relying on one ruby system test.

@ronaldlangeveld
Copy link
Author

Thank you so much for the thorough feedback @samuelpecher . Let me know if there's anything else I can do, happy to help.

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

Comments