-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[lexical-link] Fix bug when can't remove link formatting from autolink #6306
[lexical-link] Fix bug when can't remove link formatting from autolink #6306
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
size-limit report 📦
|
168a9b2
to
e575cfc
Compare
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.
LGTM overall! Can you also add some unit & E2E tests pls?
@@ -89,6 +89,11 @@ | |||
text-decoration: underline; | |||
cursor: pointer; | |||
} | |||
.PlaygroundEditorTheme__hiddenLink { |
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.
I think that instead of masking <a>
tag as "regular" text - we shall simply render "hidden link" as <span>
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.
removed that
packages/lexical-link/src/index.ts
Outdated
createDOM(config: EditorConfig): HTMLAnchorElement { | ||
if (this.__isUnlinked) { | ||
// render a link that behaves like a span | ||
const element = document.createElement('a'); |
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.
Simply use span
here? Or no wrapper all together?
e575cfc
to
30935df
Compare
hi, sure I've added an e2e test and unit tests. Also now the unlinked AutoLink node is rendered as "span" element. For that I had to update several types of the |
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.
LGTM!
This PR fixes a bug where a user cannot remove link formatting (unlink) from links created by AutoLinkNode. However, it works for links created by LinkNode. The proposal is to add a flag property in AutoLinkNode to indicate if the link was unlinked. If so, render the link as regular text, keeping styles. The hidden auto-link can become a link again via the Toolbar Link button.
Closes #5607
Test plan
Before
Screen.Recording.2024-06-13.at.15.18.13.mov
After
Screen.Recording.2024-06-13.at.15.20.36.mov