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

#4554 - Support variant monomers in Ketcher (flex mode) #5314

Merged
merged 6 commits into from
Aug 21, 2024

Conversation

rrodionov91
Copy link
Collaborator

@rrodionov91 rrodionov91 commented Aug 19, 2024

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

  • added variant monomers to model/view and deserialization

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

@rrodionov91 rrodionov91 force-pushed the 4554-variant-monomers-flex-mode branch from f0c324b to 90c15a1 Compare August 19, 2024 17:12
@rrodionov91 rrodionov91 linked an issue Aug 19, 2024 that may be closed by this pull request
@rrodionov91 rrodionov91 force-pushed the 4554-variant-monomers-flex-mode branch from 501a583 to 5887048 Compare August 20, 2024 17:47
templateId: string;
}

export interface IKetGroupNode {
type: 'group';
export interface IKetVariantMonomerNode {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it would be nice to have some common interface to encapsulate id, position and other common fields and IKetMonomerNode and IKetVariantMonomerNode to extend them with a particular type

@@ -160,7 +191,11 @@ export interface IKetMacromoleculesContentRootProperty {
}

export interface IKetMacromoleculesContentOtherProperties {
[key: string]: KetNode | IKetMonomerTemplate | IKetMonomerGroupTemplate;
[key: string]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpicking, but { [key: string]: something } might be replaced with Record<string, _something>

@@ -23,7 +25,7 @@ export class Sugar extends BaseMonomer {
}

private getValidPoint(
otherMonomer: BaseMonomer,
otherMonomer: BaseMonomer & IVariantMonomer,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This types intersection is used here and other files few times, maybe it makes sense to separate it into some util type?

return _monomer;
}

const monomer = new VariantMonomer(variantMonomerItem, position);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it's worth having some method to create variant monomer without changes in model which I implemented recently? Look for createMonomer method within this class

@rrodionov91 rrodionov91 merged commit 4dc3168 into master Aug 21, 2024
5 checks passed
@rrodionov91 rrodionov91 deleted the 4554-variant-monomers-flex-mode branch August 21, 2024 12:09
Guch1g0v pushed a commit that referenced this pull request Oct 17, 2024
- added variant monomers to model/view and serialization/deserialization
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.

Support variant monomers in Ketcher (flex mode)
2 participants