-
-
Notifications
You must be signed in to change notification settings - Fork 709
fix: Inserting link removes comment #2620
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,50 @@ | ||||||||||||||||||
| import { expect } from "@playwright/test"; | ||||||||||||||||||
| import { test } from "../../setup/setupScript.js"; | ||||||||||||||||||
| import { BASE_URL, LINK_BUTTON_SELECTOR } from "../../utils/const.js"; | ||||||||||||||||||
| import { focusOnEditor } from "../../utils/editor.js"; | ||||||||||||||||||
|
|
||||||||||||||||||
| test.beforeEach(async ({ page }) => { | ||||||||||||||||||
| await page.goto(BASE_URL); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| test.describe("Check Link Toolbar functionality", () => { | ||||||||||||||||||
| test("Should preserve existing marks when editing a link", async ({ | ||||||||||||||||||
| page, | ||||||||||||||||||
| }) => { | ||||||||||||||||||
| await focusOnEditor(page); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Type bold text | ||||||||||||||||||
| await page.keyboard.type("hello"); | ||||||||||||||||||
| await page.keyboard.press("Shift+Home"); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Make it bold via formatting toolbar | ||||||||||||||||||
| await page.waitForSelector(`[data-test="bold"]`); | ||||||||||||||||||
| await page.click(`[data-test="bold"]`); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Add link | ||||||||||||||||||
| await page.keyboard.press("Shift+Home"); | ||||||||||||||||||
| await page.waitForSelector(LINK_BUTTON_SELECTOR); | ||||||||||||||||||
| await page.click(LINK_BUTTON_SELECTOR); | ||||||||||||||||||
| await page.keyboard.type("https://example.com"); | ||||||||||||||||||
| await page.keyboard.press("Enter"); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Move cursor back onto the linked text to trigger link toolbar | ||||||||||||||||||
| await page.keyboard.press("ArrowLeft"); | ||||||||||||||||||
| await page.waitForTimeout(500); | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, let's locate and read the test file
find . -name "linktoolbar.test.ts" -type fRepository: TypeCellOS/BlockNote Length of output: 118 🏁 Script executed: # Read the test file to see lines around 33 and 44
cat -n tests/src/end-to-end/linktoolbar/linktoolbar.test.ts | head -60Repository: TypeCellOS/BlockNote Length of output: 2041 Replace hard timeouts with state-based waits Line 33 and Line 44 use Suggested change // Move cursor back onto the linked text to trigger link toolbar
await page.keyboard.press("ArrowLeft");
- await page.waitForTimeout(500);
// Click Edit link button
const editButton = page.getByText("Edit link");
await editButton.waitFor({ state: "visible" }); await page.keyboard.press("Enter");
- await page.waitForTimeout(300);
// Verify bold mark is still present on the text
const boldText = page.locator("strong a, a strong");
await expect(boldText).toBeVisible();🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| // Click Edit link button | ||||||||||||||||||
| const editButton = page.getByText("Edit link"); | ||||||||||||||||||
| await editButton.waitFor({ state: "visible" }); | ||||||||||||||||||
|
Check failure on line 37 in tests/src/end-to-end/linktoolbar/linktoolbar.test.ts
|
||||||||||||||||||
| await editButton.click(); | ||||||||||||||||||
|
|
||||||||||||||||||
| await page.keyboard.press("Control+A"); | ||||||||||||||||||
| await page.keyboard.type("https://example2.com"); | ||||||||||||||||||
| await page.keyboard.press("Enter"); | ||||||||||||||||||
|
|
||||||||||||||||||
| await page.waitForTimeout(300); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Verify bold mark is still present on the text | ||||||||||||||||||
| const boldText = page.locator("strong a, a strong"); | ||||||||||||||||||
| await expect(boldText).toBeVisible(); | ||||||||||||||||||
|
Comment on lines
+46
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: cat -n tests/src/end-to-end/linktoolbar/linktoolbar.test.ts | head -100Repository: TypeCellOS/BlockNote Length of output: 2041 Assertion validates only bold preservation, not the link edit itself The current assertion only verifies that bold formatting is preserved. The test name promises to validate that marks are preserved "when editing a link," but there's no check that the href was actually updated to Include the expected href in the selector to ensure the edit-link behavior is actually working: Suggested change- const boldText = page.locator("strong a, a strong");
- await expect(boldText).toBeVisible();
+ const boldEditedLink = page.locator(
+ 'strong a[href*="example2.com"], a[href*="example2.com"] strong',
+ );
+ await expect(boldEditedLink).toBeVisible();📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| }); | ||||||||||||||||||
| }); | ||||||||||||||||||
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.
should actually probably use style manager.createLink here no?
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.
Not exactly, since
editLinktakes an optional position whereascreateLinkalways uses the selection start position to insert the link. This is necessary for hovered links, as the editor selection isn't related to the link position in this case.We could ofc just update
createLinkto take an optional position/range, but because it's part of the editor API, I don't think we want to have anything related to ProseMirror positions there. Ideally this would be solved when we finish our own locations API.