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

Use new Emperor formatting methods #383

Merged

Conversation

ElDeveloper
Copy link
Member

Consolidates all the callbacks in a single file and implements the new methods that are being introduced here.

Note that this build is going to fail unless biocore/emperor#786 is merged.

@ElDeveloper ElDeveloper force-pushed the make-scavenge-emperor-non-embarrassing branch from 686fafe to d1a1ea0 Compare September 21, 2020 16:25
@ElDeveloper ElDeveloper changed the title Use new Emperor formatting methods [WIP] Use new Emperor formatting methods Sep 22, 2020
@ElDeveloper ElDeveloper changed the title [WIP] Use new Emperor formatting methods Use new Emperor formatting methods Sep 22, 2020
@emperor-helper
Copy link

The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv

@kwcantrell
Copy link
Collaborator

kwcantrell commented Sep 24, 2020

@ElDeveloper something odd seems to happen when playing two different animations. By that I mean first (on the emperor UI) select gradient=month and trajectory=trajectory. Play the animation. Then change gradient to day. The lines connecting the samples on the emperor plot jump around. The table shows the end of the animations. The images were taken from the linked .qzv.

Just playing day showing day after month
Screenshot from 2020-09-23 18-14-42 Screenshot from 2020-09-23 18-14-21

Here is the console.
Screenshot from 2020-09-23 18-41-39

@ElDeveloper
Copy link
Member Author

Thanks for bringing this up, I think this has to do with Emperor's animation trajectory rendering strategy. I just opened: biocore/emperor#787

Copy link
Collaborator

@kwcantrell kwcantrell left a comment

Choose a reason for hiding this comment

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

thanks @ElDeveloper! This looks good. @fedarko go ahead an merge this PR unless you have any further feedback.

Copy link
Collaborator

@fedarko fedarko left a comment

Choose a reason for hiding this comment

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

Thanks @ElDeveloper, this looks great! I have a few small suggestions for the top comment in the emperor-callbacks.js file, but this should be fine to merge as is.

empress/support_files/js/emperor-callbacks.js Outdated Show resolved Hide resolved
@@ -1,3 +1,7 @@
/*
* This File is intended to be used only with Emperor
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest adding a link to the Emperor docs on JS integration. Not sure what URL is preferred (https://github.com/biocore/emperor/blob/master/doc/source/js_integration.rst?), but would help make the code even clearer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, just added that link - which should be the most up-to-date since we currently depend on a development version of Emperor.

@fedarko
Copy link
Collaborator

fedarko commented Sep 26, 2020

Thanks all!

@fedarko fedarko merged commit db2272e into biocore:master Sep 26, 2020
@ElDeveloper
Copy link
Member Author

ElDeveloper commented Sep 26, 2020 via email

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.

4 participants