-
Notifications
You must be signed in to change notification settings - Fork 3
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
Adding short gene names to the region plot title, and toning down the color scheme #44
Conversation
…tly once LocusZoom updates are published
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.
Style changes look good and page works as intended.
The next step to UI cleanup might be to reorganize where the menus are, or how much space is allocated to a given control. (eg, I'm not sure the "anchor tissue" concept is as clear as we could wish)
But that would be a larger redraw, and what we have now is a solid incremental improvement. Good to merge as is.
|
||
<div class="container-fluid"> | ||
<div class="row"> | ||
<div id="lz-plot" class="ten columns"> |
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.
A small aside: "ten columns" is a skeleton.css construct; this page started life copied from a locuszoom example. It has no effect in bootstrap, which is what we are using for layout now. (I've pushed a change to remove that; feel free to clean up any other usages you might see)
<input type="radio" name="tss-options" autocomplete=off value="100000"> ±100kb | ||
</label> | ||
<label class="btn btn-secondary" data-toggle="tooltip" data-placement="top" | ||
title="Show eQTLs for genes with TSS in the range chr{{ chrom }}:{{ '{:,}'.format([pos - 200000, 1] | max) }}-{{ '{:,}'.format(pos + 200000) }}"> |
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.
Side thought- if we are repeating the same display transformation a lot (like range), we could add some sort of custom jinja tag/filter. This sort of trick (deduplicating code) would mostly be useful if we wanted to keep UI consistent, but it's not in any way urgent.
Thanks for the review! Agreed on the anchor concept, I think the easiest way to explain it is to create a tutorial / demo, and write it up as our application note, similar to the structure of the PheWeb paper. I didn't put in the ten columns formatting, so it's good to know the history behind these choices - I had no context for why it was there, but since I didn't know what it was I just left it alone. |
Ticket
n/a
Purpose
More updates for the UI, mainly to tone down the colors on the buttons and move parts around to make the more important components stand out more (i.e. the main association plots). Also updates the region plot's title (temporary fix until LocusZoom update is published; we will want to update it back to using the title text after statgen/locuszoom#176 is addressed).
How to test this
In the region view, change anchor gene and tissue, and add new genes and tissues to ensure correct behavior; in the single-variant view, click all the buttons to make sure they still work as expected.
Deployment / configuration notes
(none)