Style rich text editor newlines as line breaks, not paragraph breaks#10293
Conversation
…tory - Add RichTextStylesExt trait with new_with_default_line_height for comment editors - Add IndentableBlockSpacing::new() and BlockSpacings::uniform() constructors - Add style_factory field to RichTextEditorConfig/RichTextEditorView - Consolidate style_factory with initial styles in comment_editor - Compact text block spacings, 3px cursor width, remove minimum_paragraph_height Co-Authored-By: Oz <oz-agent@warp.dev>
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds custom rich text styling for code review comment editors, wires a style factory through RichTextEditorView so styles rebuild correctly after appearance/font changes, and adds related editor keybindings.
Concerns
- No blocking correctness or security concerns found.
- Two comment typos were introduced in touched code.
Verdict
Found: 0 critical, 0 important, 2 suggestions
Approve with nits
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
alokedesai
changed the title
| Container::new(footer_row) | ||
| .with_vertical_padding(8.) | ||
| .with_horizontal_padding(8.) | ||
| .with_vertical_padding(4.) |
There was a problem hiding this comment.
This just reduces the padding of the comment editor, which looks much better now that we've reduced the padding of the editor
Before:
After:
| use warpui::elements::ListIndentLevel; | ||
|
|
||
| const NOTEBOOK_LINE_HEIGHT_RATIO: f32 = 1.6; | ||
| const NOTEBOOK_LINE_HEIGHT_RATIO: f32 = 1.5; |
There was a problem hiding this comment.
Bumped this down just a tad to overall reduce the size of any given line
|
|
||
| pub const TEXT_SPACING: BlockSpacing = BlockSpacing { | ||
| margin: Margin::uniform(4.).with_right(16.), | ||
| margin: Margin::uniform(0.).with_right(16.), |
There was a problem hiding this comment.
This is the key change. By setting the margin to 0px, we just use line height.
The downside is there may be slightly less margin between different a non-text block and a text block but I've bashed this pretty extensively and I don't see any visual issues.
| margin: Margin::uniform(4.) | ||
| .with_top(12.) | ||
| .with_bottom(12.) | ||
| .with_top(4.) |
There was a problem hiding this comment.
I reduce this, the header spacing was way too big (I think the post_process step was obscuring this because we'd often remove newlines between a header and the actual text)
alokedesai
changed the title
bnavetta
left a comment
bnavetta
left a comment
There was a problem hiding this comment.
I considered fixing this deeper in the stack by not creating a new paragraph block when a user creates a newline after an existing paragraph block. That ended up being a much more complicated change than I intended, so opted for this now given priorities. Happy to keep on investigating that if you feel strongly we should go down that approach.
I think the approach here, rather than conditionally folding paragraphs together, is safest. There are a ton of assumptions in the editor around newline handling
2 participants
Description
The rich text editor would treat each newline as a paragraph break instead of line break, causing the user-visible line height to look unusually large.
This PR changes the styling of the rich text editor to more closely mimic a normal markdown editor instead of an editor like Notion.
Specifically:
0px. There's now no difference in padding between hard-wrapped and soft-wrapped lines.post_processnotebook step that strips newlines. This processing step was just wrong, the filesystem should be the source of truth. Removing newlines can change how a document reads, for ex:foo\n\n\nbaris different fromfoo\nbar.I considered fixing this deeper in the stack by not creating a new paragraph block when a user creates a newline after an existing paragraph block. That ended up being a much more complicated change than I intended, so opted for this now given priorities. Happy to keep on investigating that if you feel strongly we should go down that approach.
Testing
I did a bunch of monkey testing with AI-generated markdown. See screenshots and looms below
Left is before, right is after:
Loom
https://www.loom.com/share/7d90d47720314eaa9aba7040935b8d51?from_recorder=1&focus_title=1