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

Bug Fix: added a fix for discarding multiple color picker entries on drag #5335

Merged

Conversation

bhavyakaria
Copy link
Contributor

@bhavyakaria bhavyakaria commented Dec 5, 2023

fix: #3890

Before:

290535690-733a836e-78ea-421f-8dbd-5b81485d7d90.mov

After:

Screen.Recording.2023-12-14.at.6.57.32.PM.mp4

Copy link

vercel bot commented Dec 5, 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 Dec 9, 2023 11:43am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 9, 2023 11:43am

@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 5, 2023
@bhavyakaria
Copy link
Contributor Author

PR is ready for review.

Copy link
Member

@zurfyx zurfyx left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

@@ -186,14 +203,21 @@ function MoveWrapper({className, style, onChange, children}: MoveWrapperProps) {
move(e);

const onMouseMove = (_e: MouseEvent): void => {
setDragged(true);
updateSkipHistoryFlag(true);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a useState here or can we do with useRef? It might be easier to debug later without the side effects of having this variable is the useEffect dependency. I believe even a module variable would suffice for this case since there can only be one event in total in the entire page

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zurfyx Thanks for the review.

Agreed, useState was not necessary, have made he skipFlag to be module variable and dragged to be useRef

@bhavyakaria
Copy link
Contributor Author

PR is ready for review

Copy link
Collaborator

@ivailop7 ivailop7 left a comment

Choose a reason for hiding this comment

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

LGTM

@acywatson acywatson merged commit 617bb5f into facebook:main Dec 28, 2023
40 of 45 checks passed
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.

Bug: Selecting color on the color picker creates multiple entries on the undo stack
5 participants