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

Adding short gene names to the region plot title, and toning down the color scheme #44

Merged
merged 9 commits into from
Dec 20, 2019

Conversation

amkwong
Copy link
Collaborator

@amkwong amkwong commented Dec 20, 2019

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)

@amkwong amkwong added the enhancement New feature or request label Dec 20, 2019
@amkwong amkwong requested a review from abought December 20, 2019 04:41
Copy link
Member

@abought abought left a 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">
Copy link
Member

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) }}">
Copy link
Member

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.

@abought abought merged commit 869dd38 into master Dec 20, 2019
@amkwong
Copy link
Collaborator Author

amkwong commented Dec 20, 2019

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.

@abought abought deleted the gene-name-in-region-plot branch April 2, 2020 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants