Skip to content
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

markdown: support formatted link text #5149

Closed
wants to merge 3 commits into from

Conversation

robfig
Copy link
Contributor

@robfig robfig commented Oct 22, 2023

Fixes #5148

@vercel
Copy link

vercel bot commented Oct 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 12, 2024 0:23am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 12, 2024 0:23am

acywatson
acywatson previously approved these changes Oct 23, 2023
Copy link
Contributor

@acywatson acywatson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanк you!

@@ -176,7 +177,7 @@ function getTextSibling(node: TextNode, backward: boolean): TextNode | null {
if (!sibling) {
const parent = node.getParentOrThrow();

if (parent.isInline()) {
if (parent.isInline() && !$isLinkNode(parent)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, is there a better way than special-casing LinkNode?

This creates a dependency on @lexical/link, which is less than ideal. it might be necessary here though.

@fantactuka thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would still be a special case, but I can break the dependency by inlining the test of parent.getType() === "link" or similar.

Thinking about expanding beyond that.. would it be ok to remove the code that continues formatting spans across Element nodes & always create boundaries on either side of consecutive spans of Text Nodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been using this change in production for 6 months and been happy with it. What needs to be done to get this upstream @acywatson ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no objection to this now. If we get the CI passing again, I'll merge it

@Eomm
Copy link

Eomm commented May 9, 2024

What is missing to this PR to get it merged?

@robfig
Copy link
Contributor Author

robfig commented May 11, 2024

Rebased on main

@GermanJablo
Copy link
Contributor

Hi @robfig, seems like there are failing tests. Do you think you could review this?

@robfig
Copy link
Contributor Author

robfig commented Sep 24, 2024

Hi, I looked at the failing tests and realized that the approach I took does lead to that functionality being changed/broken (I forget which exactly). It didn't seem like something which would be easy to fix, so I lost steam

@potatowagon potatowagon added the stale-pr PRs that are closed due to staleness. Please reopen the PR if there are new updates label Nov 11, 2024
@potatowagon
Copy link
Contributor

Closing this PR due to staleness! If there are new updates, please reopen the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. stale-pr PRs that are closed due to staleness. Please reopen the PR if there are new updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Markdown import/export of formatted link text
6 participants