-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: Enable line breaks in text editor without splitBlock - Add hard… #2118
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
feat: Enable line breaks in text editor without splitBlock - Add hard… #2118
Conversation
…_break node to ProseMirror schema for <br> tag support - Implement smart Enter key handler with context-aware behavior - Add bidirectional content conversion between newlines and <br> tags - Update text editor component with proper multi-line support - Fix TypeScript errors and optimize code structure - Resolves onlook-dev#2065 as alternative to PR onlook-dev#2083
|
@Rish-it is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 233c272 in 1 minute and 57 seconds. Click for details.
- Reviewed
278lines of code in5files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/web/preload/script/api/elements/text.ts:84
- Draft comment:
Consider consolidating the newline-to-
conversion logic into a shared helper to avoid duplication across files. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. apps/web/preload/dist/index.js:12941
- Draft comment:
Typographical note: In the extractTextContent function, the replacement string using a multi-line template literal might be unintentional. If the intent is to replace
with a newline character, consider using "\n" for clarity instead of a template literal spanning multiple lines. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_m6Zf4NuUIDZOm0Fu
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
apps/web/client/src/components/store/editor/overlay/prosemirror/index.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Rish-it , this is great!
One more thing to consider is how this new change gets written to code. I've opened a PR here with an example of replacing the new newline content with a
. We can update this with whatever format we're deciding to write back to code.
One last thing to handle is that if an element actually has a <br/> or \n in between, we're able to read that in and edit.
It may require updating the conditions here in order to get the expected text content. Nice work so far, this is super complex so I'm very impressed that you got here. Can be tested by adding a <br/> in existing code and seeing how it handles.
| export function startEditingText(domId: string): EditTextResult | null { |
|
@Kitenite go through it and let me know if this till needs anymore conditions handling Key Changes:
|
|
@Rish-it sorry for the late reply, it's still missing the write to code part. Right now it's not writing the new line, perhaps needs the other PR I opened merged? |
feat: example write newline text
|
No worries at all! @Kitenite I had intentionally held off on merging your earlier PR while I was testing out this approach. We’ll likely need to incorporate parts of your changes to fully resolve the issue. I’ve just merged it and will review everything together, then update the PR after validating the expected behavior. This definitely deserves a bit more attention. Appreciate your input as always! |
Type of Change
writeToCode.mp4 |
| newContent, | ||
| }); | ||
| // Write changes back to code files | ||
| try{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't be necessary since we already handle write on text.end() but nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work! This was probably very hard to test and is a very complex issue. I've avoided it for a while for its complexity. Thank you @Rish-it :)
onlook-dev#2118) * feat: Enable line breaks in text editor without splitBlock - Add hard_break node to ProseMirror schema for <br> tag support - Implement smart Enter key handler with context-aware behavior - Add bidirectional content conversion between newlines and <br> tags - Update text editor component with proper multi-line support - Fix TypeScript errors and optimize code structure - Resolves onlook-dev#2065 as alternative to PR onlook-dev#2083 * fix: Clean up code duplication and template literal issue * add newline * refactor and few addition * writing to the code added
Description
This PR implements line break functionality in the text editor without using ProseMirror's
splitBlockcommand. The solution provides context-aware Enter key behavior that preserves the existing quick-edit workflow while enabling multi-line text editing through<br>tags.Key Implementation Details:
hard_breaknode to ProseMirror schema for semantic<br>tag support\n) and HTML break tags (<br>)Technical Benefits:
<br>tags instead of complex paragraph splittingRelated Issues
Fixes #2065
Related to #2083 (provides alternative implementation approach)
Type of Change
Testing
Manual Testing Performed:
Quick Exit Workflow (Preserved)
Multi-line Text Creation (New)
Content Conversion (New)
\n↔<br>conversion<br>tag insertionEdge Cases
Code Quality Verification:
Screenshots (if applicable)
Screen.Recording.2025-06-09.at.5.18.24.PM.mov
Additional Notes
Why This Approach Over
splitBlock:<p>with<br>tags vs multiple<p>elementsBackward Compatibility:
Important
Enables line breaks in text editor using
<br>tags, updating ProseMirror schema and content handling functions.hard_breaknode to ProseMirror schema inindex.tsfor<br>tag support.index.tsto create line breaks or exit on empty paragraphs.<br>tags intext.tsxandinsert.ts.createNodesFromContentandextractContentWithNewlinesintext.tsxhandle content conversion.createSmartEnterHandlerinindex.tsmanages Enter key behavior.updateTextContentandextractTextContentintext.tsandinsert.tshandle DOM content updates.TextEditorcomponent intext.tsxfor multi-line content handling.createElementininsert.tsto convert newlines to<br>tags.startEditingTextandstopEditingTextintext.tsfor consistent content handling.This description was created by
for 233c272. You can customize this summary. It will automatically update as commits are pushed.