-
Notifications
You must be signed in to change notification settings - Fork 1
Fix default chain, incorrect event managing with pdbe-molstar, and remove dangling items when selection changes #240
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
Conversation
…aded from the new item
…nto feature/alignment
tokland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plugin controlling code is not pretty (React code that controls an external imperative library never is), but if it works, it's still a idiomatic workflow. I added some minor comments that need no re-open, just for the record.
| const ligandId = newSelection.ligandId; | ||
| const chains = React.useMemo(() => options.pdbInfo?.chains ?? [], [options.pdbInfo?.chains]); | ||
|
|
||
| const chains = React.useMemo(() => pdbInfo?.chains ?? [], [pdbInfo?.chains]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not important, but here you are using useMemo as a way to keep the value immutable when it takes the fallback empty array, right? In that case, the typical approach is: const chains = pdbInfo?.changes || emptyArray. Where const emptyArray = [] is defined at the root level, so it's immutable.
| } | ||
|
|
||
| function pdbeMolstarSequenceEventCompletedWrapper(callback: () => void) { | ||
| return (sequence: any) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] as discussed in the other PR, when we don't really care about the type, it's safer to put "unknown"
|
Thank you @tokland for the review and your comments! |
📌 References
📝 Implementation