-
Notifications
You must be signed in to change notification settings - Fork 63
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
Updated column tooltips #1189
base: hotfix
Are you sure you want to change the base?
Updated column tooltips #1189
Conversation
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 added a couple of change suggestions. I think it's also a good idea to add tooltips for the Acpt, Rej, and Rev columns. Something to effect of "Click the thumps-up button to accept a neoantigen candidate". etc.
Also, please change the base branch of the PR to the hotfix branch.
@@ -20,6 +20,7 @@ callback <- function(hla_count, score_mode) { | |||
" 'Allele',", | |||
" 'Pos - The one-based position of the start of the mutation within the epitope sequence. 0 if the start of the mutation is before the epitope (as can occur downstream of frameshift mutations).',", | |||
" 'Prob Pos - Problematic positions within the best peptide.',", | |||
" 'Num Included Peptides - The total number of peptides for this mutation.',", |
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.
This might be a bit confusing for our users. While this does reflect the total number of peptides available in pVACview with meta-data, it's not all of the peptides resulting from the mutation. In the context of the aggregated report this is described as:
The number of included peptides according to the
--aggregate-inclusion-binding-threshold
and--aggregate-inclusion-count-limit
Here it might make sense to word it as "The number of top-scoring, unique peptides included for review". @malachig @chrisamiller do you have opinions on this wording.
gsub("7", hla_count, "for (var i = 7; i-7 < tips.length; i++) {"), | ||
gsub("7", hla_count, "$(header[i]).attr('title', tips[i-7]);"), |
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'm not 100% sure whether the numbers here need adjusting. I think it has something to do with columns we drop for display purposes but I'm not 100% sure. If you haven't run pVACview with your modifications, please do to ensure that there are no errors or warnings related to this.
I added a tooltip for
Num Included Peptides
and replaced the oldEval
tooltip with one forRef Match
.Resolves issue #1187