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

Add custom sequence component #14

Open
wants to merge 28 commits into
base: development
Choose a base branch
from
Open

Conversation

p3rcypj
Copy link

@p3rcypj p3rcypj commented Dec 20, 2024

📌 References

📝 Implementation

Added custom SequenceView component, in order to sync chains and ligands between the viewer and the sequence in molstar. In ligand view, the selected ligand should be shown at sequence, but will not trigger any chain change. On the other hand, when chain or entity is changed, it will reflect on the viewer dropdown and viceversa. If chain is not in entry chains, it will display a toast.

Sync with protvista was also fixed by using chainId on events.

@p3rcypj
Copy link
Author

p3rcypj commented Dec 20, 2024

@tokland You will see a bunch of try catch and some repeated code that I know for sure you will point at. I know that is not the best way to do it, but I will await your response on which would be the best way to do it (because I don't)

Copy link

@tokland tokland left a comment

Choose a reason for hiding this comment

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

In-line comments:

package.json Outdated Show resolved Hide resolved
src/app/index.ts Outdated Show resolved Hide resolved
src/app/index.ts Outdated Show resolved Hide resolved
src/app/index.ts Outdated Show resolved Hide resolved
src/app/index.ts Outdated Show resolved Hide resolved
src/app/index.ts Outdated Show resolved Hide resolved
src/app/index.ts Show resolved Hide resolved
src/app/subscribe-events.ts Show resolved Hide resolved
src/app/ui/sequence-wrapper.tsx Outdated Show resolved Hide resolved
src/app/ui/sequence.tsx Show resolved Hide resolved
@p3rcypj p3rcypj requested a review from tokland January 19, 2025 06:53
@p3rcypj
Copy link
Author

p3rcypj commented Jan 19, 2025

I apologize for the formatting and the extra time you had to take to review the PR for my mistake 🙏🏼

Copy link

@tokland tokland left a comment

Choose a reason for hiding this comment

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

In-line comments:

@@ -0,0 +1,750 @@
/* File from molstar, modified in order to synchronize with viewer */
/* Extracted from: Commit df3a7e5 (change access modifiers) MKampfrath, Dec 12 2022 */
/* Import with no modifications: Commit fcc0c6c (Add SequenceView component) p3rcypj, Dec 4 2024 */
Copy link

Choose a reason for hiding this comment

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

Cool, I reviewed this with: git diff fcc0c6c ./src/app/ui/sequence.tsx

Comments:

  • As we saw, there is no linter, so line length is not enforced, but let's keep it under control. There are some lines with up to 200 chars (ie. showToast), they should be converted to multi-line for readability.
  • We have some code at very high indentation levels (8? also code calling showToast). As always, by creating an abstraction (function / private method), we remove indentation levels and make things more declarative.

return <div className='msp-sequence-wrapper-non-empty'>
{children}
</div>;
}
Copy link

Choose a reason for hiding this comment

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

Add EOF

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.

2 participants