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

Fix demo.html and add CPK color assignments table to the visualizer #17

Merged
merged 2 commits into from
Mar 5, 2020

Conversation

bistaastha
Copy link
Contributor

Fixes #11

Fixes demo.html to point to HumanMine's URL in intermine-registry. Adds CPK coloring scheme based table on the bottom-right side of the visualizer.

@bistaastha
Copy link
Contributor Author

The visualizer looks like this with the table:
Screenshot from 2020-02-21 19-49-51

Copy link
Member

@heralden heralden left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! It works, but there are some things I hope you can take a look at before we merge this.

  • color_table shouldn't be in state, as it doesn't change. I would recommend placing it in a const or let at the top level (above the ColorTable component).
  • Setting the background color with inline styling is understandable, but could you move the static styling into CSS instead?
  • This is my fault as it wasn't properly mentioned in the issue, and it's now closed (Support more color operations #15): Could you make the color table contents change depending on the active coloring mode? The current colors are correct for uniform, but could you have it change to the colors in the tables below when by secondary structure is active? (You don't need to include the headers, just take the label and color columns from all 3 tables)

http:https://jmol.sourceforge.net/jscolors/#Secondary%20structure
2020-02-24-172742_831x414_scrot

Thanks again for your help.

@bistaastha
Copy link
Contributor Author

@uosl Thanks for letting me know :). I'll make the changes soon (probably tomorrow). The secondary structure in the visualiser seemed to have only the colors green and white, or was that only specific to the demo?

@heralden
Copy link
Member

You're right. It seems like pv (the lib providing the protein viewer) has a different coloring scheme for secondary structure.

I think we should still move forward with this, as we're going to need it anyways, and hopefully #19 will be a suitable update in the future.

@bistaastha
Copy link
Contributor Author

Hi @uosl! I made the required changes. There are now two tables - one for uniform coloring mode, another for coloring by secondary structure. They are conditionally used to render the table according to coloringMode passed to ColorTable.

Please let me know if this can be refined more.
Thank you!

Copy link
Member

@heralden heralden left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you for your work!

I'll ask the visualizer author, @akshatbhargava123 if he wants to review as well before merging.

@akshatbhargava123
Copy link
Member

@uosl this looks good to me, let's go ahead and merge it.

@heralden heralden merged commit 9d97013 into intermine:master Mar 5, 2020
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.

Explain what the colors mean
3 participants