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

#2584 #2750 - Selection tool: add flip and delete buttons & Rotation Tool: non-selected end of the selected bond should be the rotation center #2666

Merged
merged 26 commits into from
Jun 9, 2023

Conversation

yuleicul
Copy link
Collaborator

@yuleicul yuleicul commented May 24, 2023

Closes #2584
Closes #2750

What's new

Floating buttons

image

  • add new floating buttons for flipping and deleting structures.
  • remove rotate tool, horizontal flip tool, and vertical flip tool from the left toolbar
  • remove rotation's shortcut alt+r
  • delete rotation icon from codebase

New flip support

  • Support flipping multiple structures as a whole
  • Support flipping reaction arrows, pluses, and texts

more details:

Frame 1ketcher-flip-axis (7)

@yuleicul yuleicul linked an issue May 24, 2023 that may be closed by this pull request
@yuleicul yuleicul force-pushed the 2584-selection-tool-add-flip-and-delete-buttons branch from 017b5c7 to 278cc9d Compare May 25, 2023 15:32
@yuleicul yuleicul changed the title 2584 selection tool add flip and delete buttons #2584 Selection tool: add flip and delete buttons May 28, 2023
@yuleicul yuleicul marked this pull request as ready for review May 28, 2023 14:57
@yuleicul yuleicul requested a review from kaluginserg May 28, 2023 14:59
@yuleicul yuleicul requested a review from kaluginserg May 31, 2023 07:02
@yuleicul yuleicul requested a review from kaluginserg May 31, 2023 13:56
@KonstantinEpam23
Copy link
Collaborator

After merging #2680 into this branch, another issue occurs:
At this moment, arrows are not allowed to flip
I propose to: support flipping arrows in the scope of this pull request OR disable flipping buttons for arrows (and maybe create a new issue to support flipping)

Hi @yuleicul! I believe we do need to support flipping arrows for reactions (especially since we already support rotating them)
Flipping arrow by itself is probably a rare usecase, but if there are other structures in the selection - we cant disable the flip buttons and it might be confusing for the user if the arrows dont flip.

I would propose to support flipping arrows in the scope of this pull request, unless it is hard to implement.

@yuleicul yuleicul marked this pull request as draft June 5, 2023 10:47
@yuleicul yuleicul marked this pull request as ready for review June 6, 2023 05:46
@auto-assign auto-assign bot requested review from gairon and Nitvex June 6, 2023 05:46
@yuleicul yuleicul requested a review from kaluginserg June 6, 2023 05:46
@yuleicul yuleicul requested a review from kaluginserg June 7, 2023 04:11
@yuleicul yuleicul marked this pull request as draft June 8, 2023 06:55
@yuleicul yuleicul changed the title #2584 Selection tool: add flip and delete buttons #2584 #2750 - Selection tool: add flip and delete buttons & Rotation Tool: non-selected end of the selected bond should be the rotation center Jun 8, 2023
@yuleicul yuleicul marked this pull request as ready for review June 8, 2023 11:57
@yuleicul yuleicul requested a review from kaluginserg June 8, 2023 11:57
@kaluginserg
Copy link
Collaborator

@yuleicul thanks for your work, it is a good solution.
I saw you've started working on additional functionality for choosing the rotation center....
Could we please do it in a separate PR?
It is not a good practice to have long-live PRs and also it is not a good practice to have PR with various changes.
Why? Using small pull requests can lead to more efficient code reviews, faster feedback, fewer merge conflicts, and a more stable and maintainable codebase.

@Nitvex Nitvex merged commit 4d491ab into master Jun 9, 2023
@Nitvex Nitvex deleted the 2584-selection-tool-add-flip-and-delete-buttons branch June 9, 2023 13:19
@yuleicul
Copy link
Collaborator Author

yuleicul commented Jun 9, 2023

@kaluginserg Thanks for your review! Actually, the reason why I close two issues in this PR is because the two issues have overlapped code, if I open separate PRs, it may be hard to resolve conflicts.

But anyway, thanks for your kind reminder, I'll pay more attention next time.

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