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.

Sign up for GitHub

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

Jump to bottom

BugFix: Prevent insertion of link of collapsed selection #5346

Closed
wants to merge 15 commits into from
Closed

BugFix: Prevent insertion of link of collapsed selection #5346

wants to merge 15 commits into from

Conversation

bhavyakaria
Copy link
Contributor

@bhavyakaria bhavyakaria commented Dec 6, 2023 •
edited
Loading

fix: #5305

@vercel Vercel
Copy link

vercel bot commented Dec 6, 2023 •
edited
Loading

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 Nov 26, 2024 5:36pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 26, 2024 5:36pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 6, 2023
@bhavyakaria
Copy link
Contributor Author

@acywatson wanted your comments on the following behaviour, is this expected?

Current Behaviour

  • Select text
  • Click on insert link button
  • Default link with https:// gets created and then user has to click on edit icon to update the link

Shouldn't on clicking insert link button, it should be showing the edit view where user directly inserts the link and saves?Rather than having to clink on edit button to insert the link.

Attached the video for reference

Screen.Recording.2023-12-07.at.3.15.53.AM.mov

@acywatson
Copy link
Contributor

Shouldn't on clicking insert link button, it should be showing the edit view where user directly inserts the link and saves?Rather than having to clink on edit button to insert the link.

Yes - the way it currently works is the most annoying thing in the entire playground app to me. You are welcome to change it in a different PR.

@bhavyakaria bhavyakaria marked this pull request as draft December 7, 2023 07:14
@bhavyakaria bhavyakaria marked this pull request as ready for review December 15, 2023 14:22
ivailop7
ivailop7 previously approved these changes Dec 27, 2023
View reviewed changes
@ivailop7
Copy link
Collaborator

Leaving to @acywatson on how to proceed with this one.

@acywatson
Copy link
Contributor

Leaving to @acywatson on how to proceed with this one.

Looks like we have a failing test to fix. Make sure it's not causing a regression.

acywatson
acywatson requested changes Dec 28, 2023
View reviewed changes
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.

See comment re: test

@bhavyakaria bhavyakaria requested a review from Fetz as a code owner April 20, 2024 09:52
@bhavyakaria
Copy link
Contributor Author

@acywatson a bit late but have fixed the UT 😅

@bhavyakaria bhavyakaria requested a review from acywatson April 20, 2024 13:15
@ivailop7
Copy link
Collaborator

ivailop7 commented Jul 3, 2024

This introduces an issue where if try to remove an existing link when my collapsed selection is in inside the link, it blocks the toggling off of the link.

@bhavyakaria
Copy link
Contributor Author

@ivailop7 thanks for pointing out, will fix it

@bhavyakaria bhavyakaria requested a review from ivailop7 September 6, 2024 17:30
@bhavyakaria
Copy link
Contributor Author

@ivailop7 Have fixed the delete link issue.

etrepum
etrepum reviewed Nov 26, 2024
View reviewed changes
Copy link
Collaborator

@etrepum etrepum left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This seems like a good change, but I don't think it passes the current e2e test suite (probably because the tests should be updated accordingly). @acywatson might be worth removing the request changes flag so that when/if the fixes go in here we can merge it without waiting for that flag to clear

@etrepum etrepum added the stale-pr PRs that are closed due to staleness. Please reopen the PR if there are new updates label Dec 2, 2024
@etrepum
Copy link
Collaborator

etrepum commented Dec 2, 2024

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

@etrepum etrepum closed this Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etrepum etrepum etrepum left review comments

@zurfyx zurfyx Awaiting requested review from zurfyx zurfyx is a code owner

@fantactuka fantactuka Awaiting requested review from fantactuka fantactuka is a code owner

@Fetz Fetz Awaiting requested review from Fetz Fetz is a code owner

@acywatson acywatson Awaiting requested review from acywatson acywatson is a code owner

@Sahejkm Sahejkm Awaiting requested review from Sahejkm Sahejkm is a code owner

@potatowagon potatowagon Awaiting requested review from potatowagon potatowagon is a code owner

@ivailop7 ivailop7 Awaiting requested review from ivailop7 ivailop7 is a code owner

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR stale-pr PRs that are closed due to staleness. Please reopen the PR if there are new updates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Collapsed selections expand to nearest HTML tags on link insertion
5 participants
@bhavyakaria @acywatson @ivailop7 @etrepum @facebook-github-bot