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

Merge preview of abbreviated structures #2939

Closed
Nitvex opened this issue Jul 20, 2023 · 3 comments · Fixed by #3068
Closed

Merge preview of abbreviated structures #2939

Nitvex opened this issue Jul 20, 2023 · 3 comments · Fixed by #3068
Assignees
@KonstantinEpam23
Labels
feature request Priority: High
Milestone
Ketcher 2.14.0-rc.1

Comments

@Nitvex
Copy link
Collaborator

Nitvex commented Jul 20, 2023 •
edited by Zhirnoff
Loading

Once abbreviated structure is hovered over an atom or bond, the text label (like benzene) should be hidden and user should be shown a preview of how the structure will be merged with the existing one if they click left mouse button.

Acceptance criteria:
– Show a preview of structure with opacity to make it visually distinguishable
– Hide structure label, when mouse is hovered over structure

#184895577
@Nitvex Nitvex added this to the Ketcher 2.14.0-rc.1 milestone Jul 20, 2023
@Nitvex Nitvex self-assigned this Jul 20, 2023
@yuleicul yuleicul assigned yuleicul and unassigned Nitvex Jul 20, 2023
@yuleicul
Copy link
Collaborator

yuleicul commented Jul 28, 2023 •
edited by KonstantinEpam23
Loading

Dev notes for myself:

  • extract related function in mouseup, refer it in mousemove
  • nothing is pushed to historystack
  • use [map]Changed to show opacity
  • ignore preview structure when inserting a new one on mouseup
  • do not show preview for salts and solvents

@KonstantinEpam23
Copy link
Collaborator

Currently some structure can be merged "inside" each other. For example in case of benzene rings - we can merge two rings "on top of each other" .
If we have two benzene rings connected by one bond and hover with another benzene ring template over this connection bond:
Screenshot 2023-08-02 at 11 56 43

If we now click on that position - the new benzene ring template will be merged inside the two rings:
Screenshot 2023-08-02 at 11 59 35

What should be the correct behavior for showing the preview in this case? Should all the changed bonds and atoms be highlighted with reduced opacity in this case?

KonstantinEpam23 added a commit that referenced this issue Aug 7, 2023
@KonstantinEpam23
#2939: Merge preview of abbreviated structures
4e03539
KonstantinEpam23 added a commit that referenced this issue Aug 7, 2023
@KonstantinEpam23
#2939: Merge preview of abbreviated structures: incorrect type bond
e59c20e
@KonstantinEpam23 KonstantinEpam23 linked a pull request Aug 8, 2023 that will close this issue
2939 merge preview of abbreviated structures #3067
Closed
8 tasks
KonstantinEpam23 added a commit that referenced this issue Aug 9, 2023
@KonstantinEpam23
#2939: Merge preview of abbreviated structures
1c83873
KonstantinEpam23 added a commit that referenced this issue Aug 9, 2023
@KonstantinEpam23
#2939: Merge preview of abbreviated structures
77f1678
@KonstantinEpam23 KonstantinEpam23 mentioned this issue Aug 9, 2023
Merged
8 tasks
KonstantinEpam23 added a commit that referenced this issue Aug 9, 2023
@KonstantinEpam23
#2939: Update tests
08ead3c
KonstantinEpam23 added a commit that referenced this issue Aug 9, 2023
@KonstantinEpam23
#2939: Fix timeout issues
92a43e8
KonstantinEpam23 added a commit that referenced this issue Aug 9, 2023
@KonstantinEpam23
#2939: Fix test screenshots
551da77
@Zhirnoff
Copy link
Collaborator

Zhirnoff commented Aug 9, 2023

Test cases are being created.

KonstantinEpam23 added a commit that referenced this issue Aug 10, 2023
@KonstantinEpam23
#2939: Fix tests, fix incorrect variable
431fdc3
KonstantinEpam23 added a commit that referenced this issue Aug 10, 2023
@KonstantinEpam23
#2939: Trigger tests
c51d2fb
KonstantinEpam23 added a commit that referenced this issue Aug 10, 2023
@KonstantinEpam23
#2939: Update tests
f28b1ea
KonstantinEpam23 added a commit that referenced this issue Aug 10, 2023
@KonstantinEpam23
#2939: Fix timeout issues
163e9c0
KonstantinEpam23 added a commit that referenced this issue Aug 10, 2023
@KonstantinEpam23
#2939: Fix test screenshots
17a795a
KonstantinEpam23 added a commit that referenced this issue Aug 10, 2023
@KonstantinEpam23
#2939: Fix tests, fix incorrect variable
87f7658
KonstantinEpam23 added a commit that referenced this issue Aug 10, 2023
@KonstantinEpam23
#2939: Trigger tests
6710dde
KonstantinEpam23 added a commit that referenced this issue Aug 10, 2023
@KonstantinEpam23
#2939: resolve conflicts
ba341ff
KonstantinEpam23 added a commit that referenced this issue Aug 11, 2023
@KonstantinEpam23
#2939: Fix review comments
ba8a0ac
KonstantinEpam23 added a commit that referenced this issue Aug 14, 2023
@KonstantinEpam23
#2939: Fix review comments again
fb053d2
Nitvex added a commit that referenced this issue Aug 14, 2023
@Nitvex
#2939 – updated screenshots, added fixme for failing tests
28f305e
Nitvex added a commit that referenced this issue Aug 15, 2023
@KonstantinEpam23 @Nitvex
#2939 - Merge preview of abbreviated structures (#3068)
266db1c 
* 2939: Merge preview of abbreviated structures

* #2939: Update tests

* #2939: Fix timeout issues

* #2939: Fix test screenshots

* #2939: Fix tests, fix incorrect variable

* #2939: Trigger tests

* #2939: resolve conflicts

* #2939: Fix review comments

* #2939: Fix review comments again

* #2939 – updated screenshots, added fixme for failing tests

---------

Co-authored-by: Nikita_Vozisov <Nikita_Vozisov@epam.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@KonstantinEpam23 KonstantinEpam23

Labels
feature request Priority: High
Projects
None yet
Milestone
Ketcher 2.14.0-rc.1
4 participants
@yuleicul @Nitvex @Zhirnoff @KonstantinEpam23