Chip-to-block resize should split the surrounding sentence#136
Chip-to-block resize should split the surrounding sentence#136ihordubas99 wants to merge 3 commits into
Conversation
judithroth
left a comment
There was a problem hiding this comment.
It works as expected ✨
Please add browser test for this.
Cover 3 cases:
- Work Package - text
- text - Work Package - text
- text - Work Package
Please always add tests 🙂
| await expect.element(page.getByTestId('block-card')).toBeVisible(); | ||
| await expect.element(page.getByText('Fix login bug')).toBeVisible(); | ||
| await expect.element(page.getByText('Hello')).toBeVisible(); | ||
| await expect.element(page.getByText('World')).toBeVisible(); |
There was a problem hiding this comment.
Thanks for adding these tests! However, they will surely still be green even if the the splitting into paragraphs does not work. Because it does not check for newlines. So, like this the tests are not really helpful unfortunately.
Please try to improve these tests and check for newlines e.g. in this case for "Hello\n<card content>\nWorld" or something similar.
95add29 to
f276ec4
Compare
There was a problem hiding this comment.
Pull request overview
Fixes inline-to-block chip resize so the surrounding paragraph is split at the chip's position rather than leaving pre/post text merged.
Changes:
handlePromoteToBlocknow slices the host block's inline content intocontentBefore/contentAfterand either updates the original paragraph or replaces it depending on whether preceding content exists.- New
placeAfterContenthelper inserts a trailing paragraph for following content and otherwise falls back tomoveCursorAfterBlock, with cursor placement at the start of the trailing paragraph. - Adds browser integration tests for the three split scenarios (
WP - text,text - WP - text,text - WP) plus a newinsertInlineChipViaHashWithTextBeforetest helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/hooks/useInlineWpEvents.ts | Split surrounding content around chip on promote-to-block; new placeAfterContent helper handles trailing paragraph + cursor. |
| test/helpers/editorHelpers.ts | Adds insertInlineChipViaHashWithTextBefore helper that types prefix text then selects a WP via hash menu. |
| test/lib/components/integration/editor.conversion.browser.test.tsx | Adds three integration tests asserting DOM order/containment for split scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [afterParagraph] = editor.insertBlocks( | ||
| [{ type: 'paragraph', content }], | ||
| anchorBlockId, | ||
| 'after' | ||
| ); | ||
| requestAnimationFrame(() => { | ||
| editor.focus(); | ||
| editor.setTextCursorPosition(afterParagraph.id, 'start'); | ||
| }); |
There was a problem hiding this comment.
Here insertBlocks always returns a non-empty result - we're inside content.length > 0 passing a non-empty array. A silent return would hide a bug rather than expose it, so skipping this one intentionally.
Ticket
https://community.openproject.org/projects/communicator-stream/work_packages/74978
What are you trying to accomplish?
When resizing an inline work package chip to block size, the surrounding text was not split at the chip's position. The entire paragraph remained intact with only the chip removed, leaving text before and after merged on one line.
After this fix, promoting a chip to block correctly splits the sentence: text before the chip stays in the original paragraph, the block chip is inserted on its own line, and text after the chip continues in a new paragraph below.
What approach did you choose and why?
handlePromoteToBlocknow finds the chip's index in the inline content array and slices it intocontentBeforeandcontentAfter. Depending on whether there is content before the chip, it either updates the original block orreplaces it entirely to avoid leaving an empty paragraph. A small
placeAfterContenthelper handles cursor placement and conditionally inserts the trailing paragraph only when there is content to place, preventing a spurious empty block when the chip was at the end of a sentence.No alternative approaches were considered - the fix is a straightforward extension of the existing editor mutation pattern used throughout the file.
Merge checklist