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

#4880 – Adjust previews – add APs, show IDT aliases for previews in sequence mode, increase debounce time #5160

Conversation

svvald
Copy link
Collaborator

@svvald svvald commented Jul 24, 2024

How the feature works? / How did you fix the issue?

(Screenshots, videos, or GIFs, if applicable)

Check list

  • unit-tests written
  • e2e-tests written
  • documentation updated
  • PR name follows the pattern #1234 – issue name
  • branch name doesn't contain '#'
  • PR is linked with the issue
  • base branch (master or release/xx) is correct
  • task status changed to "Code review"
  • reviewers are notified about the pull request

packages/ketcher-macromolecules/src/Editor.tsx Outdated Show resolved Hide resolved
packages/ketcher-macromolecules/src/Editor.tsx Outdated Show resolved Hide resolved
return null;
}

if (presetName.includes('MOE')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why rules applied only for MOE? I believe it should be for all presets

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is the following requirement for these specific presets:
image
All the other presets should just have their base both on canvas and in the library. I don't like handling it by name though, maybe we could handle it in a better manner later

const { R1, R2 } = attachmentPointsToBonds;
// Handle phosphate exclusively
if (base === 'Phos') {
if (R1 !== null && R1.firstMonomer instanceof Sugar) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sugar can be the second monomer in bond if user connected it from right to left. Probably need another check here

Copy link
Collaborator Author

@svvald svvald Jul 31, 2024

Choose a reason for hiding this comment

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

According to the requirement, we need to check only the specific case when sugar and phosphate have left-to-right connection.
4.2.3. If the phosphate is the last monomer in the chain, but the second to last is not a sugar/nucleozide, the alias should be: 3Phos.
It doesn't matter, if there is a sugar after phosphate, it will be covered by the general rules for monomers

@svvald svvald force-pushed the 4880-change-previews-for-hovering-over-the-monomer-in-the-library-and-on-the-canvas branch 2 times, most recently from 0073965 to 91f462e Compare July 31, 2024 20:30
@rrodionov91 rrodionov91 force-pushed the 4880-change-previews-for-hovering-over-the-monomer-in-the-library-and-on-the-canvas branch from ae4fd6a to f38b868 Compare August 6, 2024 14:18
@rrodionov91 rrodionov91 force-pushed the 4880-change-previews-for-hovering-over-the-monomer-in-the-library-and-on-the-canvas branch from f38b868 to 42826a9 Compare August 6, 2024 15:18
Copy link
Collaborator

@AlexeyGirin AlexeyGirin left a comment

Choose a reason for hiding this comment

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

Approved

@rrodionov91 rrodionov91 merged commit d10e30d into master Aug 6, 2024
5 checks passed
@rrodionov91 rrodionov91 deleted the 4880-change-previews-for-hovering-over-the-monomer-in-the-library-and-on-the-canvas branch August 6, 2024 17:13
Guch1g0v pushed a commit that referenced this pull request Oct 17, 2024
…equence mode, increase debounce time (#5160)

- added changes according new requirements for IDT aliases
- added util function to add H to O leaving group
- fixed idt aliases in preview for unresolved monomers
- updated screenshots

---------

Co-authored-by: Roman Rodionov <roman_rodionov@epam.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants