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

#2276 Click & drag an 'Atom' on FG, Salts and Solvents, FG connected with Atom forms many atoms #2577

Merged
merged 4 commits into from
May 16, 2023

Conversation

VasilevDO
Copy link
Collaborator

Closes #2276
Changes:

  1. Atom tool "click and drag" action works as expected now
  2. If the user clicks and drags an atom out of the original target, and then hovers back over the original target, the bond action will revert (after the 'mouseup' event, it will trigger a replace action)

@Nitvex Nitvex requested review from a user and yuleicul May 5, 2023 14:06
Copy link
Collaborator

@yuleicul yuleicul left a comment

Choose a reason for hiding this comment

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

Hi @VasilevDO , it looks really cool to me in the sense that the bond-adding action will be reverted after hovering back! 👍

Unfortunately, I found a small problem which seems not to behave correctly, according to

Expected behavior
...
For Salt and Solvents: Atom appears where the left mouse button was released without a connection
...

code-review-2276

if (dragCtx.item.id === ci.id) {
if (
ci.map === 'functionalGroups' &&
FunctionalGroup.isContractedFunctionalGroup(ci.id, functionalGroups)
Copy link
Collaborator

Choose a reason for hiding this comment

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

that's fun that I have the same logic in my PR :) I think we need to extract this into a separate function and reuse it.
https://github.com/epam/ketcher/pull/2580/files#diff-ee62e3d2a7eb8e9230e2d13e3cfe44e4ea9ea805a61f971355d38aab66dda856R62


const ciFunctionalGroupName =
ci?.map === 'functionalGroups'
? molecule.functionalGroups.get(ci?.id)?.name
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we write here "ci?.map === 'functionalGroups' && molecule.functionalGroups.get(ci?.id)?.name"?

@yuleicul yuleicul self-requested a review May 12, 2023 03:54
@yuleicul
Copy link
Collaborator

yuleicul commented May 12, 2023

Just a kind reminder, next time please remember to click 're-request review' after your code is ready again 😃

image

@Nitvex Nitvex merged commit d399ad2 into master May 16, 2023
@Nitvex Nitvex deleted the #2276-atom-tool-click-and-drag branch May 16, 2023 12:55
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.

Click & drag an 'Atom' on FG, Salts and Solvents, FG connected with Atom forms many atoms
4 participants