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

Removed additional FloatingLinkEditorPlugin mouse listeners #4057

Merged

Conversation

ChronicLynx
Copy link
Contributor

@ChronicLynx ChronicLynx commented Mar 8, 2023

Issues this caused:

  • Could not highlight text in link editor with mouse
  • Sometimes needed to click twice to activate link editor buttons
  • If you were editing a link over / in a table it would cause a multitude of odd behavior (such as highlighting the table or the link editor bouncing around)

Pre-Fix

Screen.Recording.2023-03-08.at.9.28.09.AM.mov

After-Fix

Screen.Recording.2023-03-08.at.9.29.24.AM.mov

Reference: #4013

@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 Mar 8, 2023
@zurfyx
Copy link
Member

zurfyx commented Mar 8, 2023

Thanks for the contribution! Can you expand on the impact that this has on the floating editor? I would rather have a fix to the link plugin than breaking part of its functionality to fix the rest

@vercel
Copy link

vercel bot commented Mar 8, 2023

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

Name Status Preview Comments Updated
lexical ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 8, 2023 at 4:25PM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 8, 2023 at 4:25PM (UTC)

@ChronicLynx
Copy link
Contributor Author

Thanks for the contribution! Can you expand on the impact that this has on the floating editor? I would rather have a fix to the link plugin than breaking part of its functionality to fix the rest

I've uploaded some video showing a demo of both. It seems that the #4013 bug is not showing up, whether that be through my changes after #4013 or through some combination of my changes and the remaining changes from that branch I'm not 100% sure.

@thegreatercurve has also investigated this and thinks we can probably remove these listeners if it doesn't seem to bring back the selection bug. Try it out through Vercel and see if you can reproduce the #4013 issue. I could not during my limited testing.

@zurfyx
Copy link
Member

zurfyx commented Mar 8, 2023

Thanks, LGTM! Tbh, I would disable the link hover behavior or non-collapsed selection (or improve the logic behind its trigger). This is no good. But that's a separate issue

Screen.Recording.2023-03-08.at.6.17.44.PM.mov

Edit: submitted a separate issue #4064

@zurfyx zurfyx merged commit 26bab25 into facebook:main Mar 8, 2023
@LuciNyan
Copy link
Contributor

LuciNyan commented Mar 9, 2023

It looks great! 👍

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants