-
Notifications
You must be signed in to change notification settings - Fork 6
min & max stoichiometry labels #212
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
complexviewer v2.0.0-beta
fix for PRO chain and isoform annotations
superfam fix
{ | ||
"ac": "CPX-37", | ||
"name": "CPX-37", | ||
"url": "https://www.ebi.ac.uk/complexportal/complex/CPX-37" |
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.
There's a small bug in complex portal (which I have fixed but not deployed yet) causing the min and max stoichiometry not being set properly when the min is 0.
The correct version of this file is
CPX-37.json
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.
yes, the min/max label appears for the correct version of the file
participant.setPosition(-500, -500); | ||
this.proteinUpper.appendChild(participant.upperGroup); | ||
} | ||
participant.setPosition(-500, -500); |
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.
I just got this code locally and I tried to run it, and this is causing errors for complex CPX-5602
app.js:254 Uncaught TypeError: Failed to execute 'appendChild' on 'Node': parameter 1 is not of type 'Node'.
at App.init (app.js:254:1)
at App.readMIJSON (app.js:489:1)
at index.html?_ijt=vs1f11ftplrbgklh33n43qk7rd&_ij_reload=RELOAD_ON_SAVE:175:16
at d3.v3.min.js:1:9203
at Object.<anonymous> (d3.v3.min.js:1:9034)
at Object.t (d3.v3.min.js:1:740)
at XMLHttpRequest.i (d3.v3.min.js:1:7909)
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.
odd, for me CPX-5602 is working,
i tested in FireFox and Chrome
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.
it's like you don't have the most recent version of complex.js
the new version of complex.js should initialise this.upperGroup
on line 12
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.
You're right, I forgot those changes. It is working now. The only thing is that because we don't have labels for complexes, when we have stoichiometry, it's just the stoichiometry being displayed.
So, if we change the code (in line 10 of complex.js) to
const complexIdMatch = interactorRef.match(/^.*(CPX-[0-9]+).*$/);
const complexId = complexIdMatch ? complexIdMatch[1] : "";
this.init(id, app, interactor, complexId);
Then we have the complex id of subcomplexes, and it would look like this
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.
Yes, i mentioned in the issue that "you could now also have similar labels for complexes giving their names if you wanted".
I thought it might be perceived as cluttered things up, but if you prefer this then lets do it like that.
Like this then, with complex names shown?
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.
I have some other trivial changes in my dev branch - removing comments, linting and also added:
setAllLinkCoordinates() {
this.setLabelPosition();
super.setAllLinkCoordinates();
}
to the Complex class, i think that is neater than way i had it before.
You can see the changes here: colin-combe/ComplexViewer@master...dev
Do you want me to add these changes to this pull request?
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.
I just had a look at the changes and they mostly seem like cleaning up and it's not that many. I would say add them to this PR.
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.
they are added to this PR.
This PR can probably be merged and the new version deployed on npm?
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.
can also mark as a release on GH which we (I) haven't done for a while
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.
Yeah, we can. Latest, and only, one is 2.2.0, in November 2022. Maybe it's time to mark another released version.
#19