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

#2060: Don't break the selection if the user's cursor goes beyond the canvas #2071

Conversation

St-Permiakov
Copy link
Collaborator

Now selection tool does not break when cursor leaves canvas.
Fixes #2060

Copy link
Collaborator

@Nitvex Nitvex left a comment

Choose a reason for hiding this comment

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

I've checked this PR locally and found that tools, which have multiple options (for example, selections and bonds) don't work from first time, if click on left panel.
You need to perform at least, two clicks in order to open, for example, panel for selection tool:
image
Could you, please, have a look?

@St-Permiakov St-Permiakov force-pushed the #2060-dont-break-the-selection-if-the-users-cursor-goes-beyond-the-canvas branch from eaa2bff to 35c91bb Compare January 10, 2023 10:11
@St-Permiakov St-Permiakov force-pushed the #2060-dont-break-the-selection-if-the-users-cursor-goes-beyond-the-canvas branch from 35c91bb to 838bf08 Compare January 10, 2023 14:42
@Nitvex Nitvex merged commit 248f71d into master Jan 10, 2023
@Nitvex Nitvex deleted the #2060-dont-break-the-selection-if-the-users-cursor-goes-beyond-the-canvas branch January 10, 2023 15:31
{ target: clientArea, eventName: 'mousedown' },
{ target: document, eventName: 'mousemove' },
{ target: document, eventName: 'mouseup' },
{ target: document, eventName: 'mouseleave' },
Copy link
Collaborator

@yuleicul yuleicul Jan 20, 2023

Choose a reason for hiding this comment

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

Hi 😃 seems it causes a small bug: pasted structure is here to stay when the mouse is moving outside the canvas.

Steps to Reproduce

  1. Cut or copy a structure
  2. Ctrl+v to paste it on the canvas
  3. Move the mouse to the toolbar

Actual behavior
The pasted structure is still here
ketcher-paste-bug-07

Expected behavior
The pasted structure disappears. The following GIF is created on v2.7.0 RC test remote version.
ketcher-paste-bug-08

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we have explicit requirements to remove it when over a toolbar?
Why should we use controversial logic for different tools? We do not break selection when going over toolbars, why should we break copy-paste action?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right. Actually, sometimes I'm a bit confused about some controversial logic. Thanks for your fast reply :)

Copy link
Collaborator

@yuleicul yuleicul Jan 20, 2023

Choose a reason for hiding this comment

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

And please check out this one. The hand tool is still following the cursor when moving outside the canvas.

ketcher-paste-bug-09

I'm not sure if it's caused by that line of code, but I guess it's because the mouseleave event is not triggered. The following code in ketcher-react/src/script/ui/views/components/StructEditor/StructEditor.jsx may help.

case 'leave':
  this.editorRef.current.classList.remove(classes.enableCursor)
  this.setState({
    enableCursor: false
  })
  break

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that does look weird. Thanks for noticing.
I think it deserves its own issue on the board. I'll look into it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't break the selection if the user's cursor goes beyond the canvas
4 participants