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

Sequence viewer app improvements #87

Merged
merged 18 commits into from
Jan 2, 2019

Conversation

shammamah-zz
Copy link
Contributor

Addresses #58.

@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-87 December 18, 2018 19:39 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-87 December 18, 2018 20:01 Inactive
Copy link
Contributor

@mtwichan mtwichan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-87 December 19, 2018 15:39 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-87 December 19, 2018 15:56 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-87 December 19, 2018 16:24 Inactive
@shammamah-zz shammamah-zz mentioned this pull request Dec 19, 2018
Copy link
Contributor

@nchtra nchtra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The coverage data that's shown in the main sequence container (left panel) seems to be the same for all sequences. Should it be disabled for sequences where it does not apply?
  2. I wonder if the "Translated from" in the bottom of the middle panel should have just DNA and RNA as one would not need to translate protein to protein
  3. The output (right panel) of this translation, from DNA to protein for example, is shown as "Protein translated from DNA to" followed by the translated sequence. This seems a little confusing. I would suggest removing the "to" and changing this to "Protein translated from DNA: " followed by the actually sequence translation.
  4. I like the colors used to demarcate the different sections in the right panel. The only thing I see is that the colors take some focus off the content shown, so it'd probably be good to make the colors more transparent :)

@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-87 December 20, 2018 17:04 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-87 December 20, 2018 21:19 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-87 December 20, 2018 21:34 Inactive
@mkcor
Copy link
Contributor

mkcor commented Dec 21, 2018

I don't know what you think @VeraZab, but I would suggest rebasing on master (from https://github.com/plotly/dash-bio/branches I'm seeing that this branch is 5 commits behind master).

@mkcor
Copy link
Contributor

mkcor commented Dec 21, 2018

@shammamah If you need help rebasing, I'm more than happy to help out! I admit I would prefer sitting side by side, but we can work it out remotely too.

@VeraZab
Copy link
Contributor

VeraZab commented Dec 21, 2018

@mkcor note that this branch had master merged into it, so maybe a rebase could be messy..

@mkcor
Copy link
Contributor

mkcor commented Dec 21, 2018

@mkcor note that this branch had master merged into it, so maybe a rebase could be messy..

Thanks, @VeraZab! In the meantime, I looked up these guidelines for contributing https://github.com/plotly/plotly.js/blob/master/.github/PULL_REQUEST_TEMPLATE.md#features-bug-fixes-and-others so I respect the preference for merging master rather rebasing once the PR is submitted (the issue doesn't arise so much if PR are kept small and fast; because then you rebase before pushing, and your PR gets reviewed before much would happen on master).

Copy link
Contributor

@Bachibouzouk Bachibouzouk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I select Myosin 0 to 21 and choose translate from RNA or DNA, the translated protein sequence does not change, it is normal?

Otherwise I see that you implemented the changes requested by @matthewchan15 :), good job!

💃

)
def update_sel(slider_value, selOrCov, color):
def update_sel(slider_value, _, selOrCov, color, slider_input,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selOrCov is JS ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JS style ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃

@Bachibouzouk
Copy link
Contributor

@matthewchan15 Could you approve the changes @shammamah made upon your request?

@shammamah-zz shammamah-zz merged commit b389a45 into master Jan 2, 2019
@shammamah-zz shammamah-zz deleted the sequence-viewer-app-improvements branch January 2, 2019 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants