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

HACK: Remove "Open in Vega Editor" from volatility for now #154

Merged
merged 7 commits into from
Jun 3, 2020

Conversation

Oddant1
Copy link
Member

@Oddant1 Oddant1 commented May 27, 2020

band aids #153
band aids #155

  1. The plot in vega no longer automatically resizes when the window resizes. I don't know if the vega feature itself is broken now, or if something just broke our previous usage of it, but auto resizing the plot on window resize was just causing the plot to grow wider and wider and wider to unreasonable widths.
  2. The plot outside of the vega editor is now too wide. This has to do with the autosize being changed from fit-x to pad. On fit-x it was impossible to effectively resize the plot within the editor. I think if I just reduce the default width this issue may be resolved.
  3. This was happening prior to this PR, but the controls on the bottom of the plot in the editor are all smashed together. I'm not sure why. No matter how much width you give the plot they don't seem to separate.

EDIT: In regards to number 2, the plot being too wide or not outside the vega editor is now entirely dependent on monitor resolution. This is obviously not ideal.

@Oddant1 Oddant1 self-assigned this May 27, 2020
@Oddant1 Oddant1 added the stat:DO-NOT-MERGE Please do not merge this until this label has been removed. label May 27, 2020
@thermokarst thermokarst self-requested a review May 27, 2020 21:12
@Oddant1 Oddant1 marked this pull request as draft May 27, 2020 21:12
@Oddant1
Copy link
Member Author

Oddant1 commented May 28, 2020

Maybe the answer, at least in the short term, is to leave the autosize on fit-x and not have the text box there to change the width. This also allows us to leave the resize on window size change which is definitely a good thing. I'm really not finding any way to keep things sized nicely and consistently while also allowing the user to edit the width.

@thermokarst
Copy link
Contributor

while also allowing the user to edit the width.

We aren't really looking to allow that - the textbox is just a by-product of how we were doing dynamic sizing via signal. Once upon a time that was the best way to handle dynamic resize in vega. I think that some time later in the 4.x or 5 series of vega they added new options to vega for including a signal directly in the autosize attribute. I bet there's just a better, more succinct way to do this, now.

@Oddant1
Copy link
Member Author

Oddant1 commented May 28, 2020

@thermokarst in that case, my life just got a whole lot easier. Do we still want to leave all those other modifiable settings down there? They don't seem to be breaking anything. And I'm pretty sure I can ditch that textbox pretty easily now that it's not useful.

@thermokarst
Copy link
Contributor

I think we need everything listed below, except width:

Screen Shot 2020-05-28 at 11 45 35 AM

@Oddant1
Copy link
Member Author

Oddant1 commented May 28, 2020

211098c This should size the plot pretty well overall. I'll look into making it happen without that textbox. Absolute worst case I can call it something like "width (don't change)" I suppose. Or "width (set autosize type to 'pad' before changing)" those get a bit too long though.

EDIT: Making the name that long actually seriously breaks things 😂

@Oddant1 Oddant1 marked this pull request as ready for review May 28, 2020 19:11
@Oddant1
Copy link
Member Author

Oddant1 commented May 28, 2020

f02690b Turns out you can just remove the binding to the textbox and it still works just fine.

@Oddant1 Oddant1 removed the stat:DO-NOT-MERGE Please do not merge this until this label has been removed. label May 28, 2020
@Oddant1 Oddant1 assigned thermokarst and unassigned Oddant1 May 28, 2020
Copy link
Contributor

@thermokarst thermokarst left a comment

Choose a reason for hiding this comment

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

Thanks @Oddant1! I took a closer look today, and Vega 5 is going to require a little bit of engineering effort, which we probably don't have much time for before the release. Can you please:

  • update this PR to remove the "open in vega editor" button from the template
  • file an issue in this repo that puts "upgrade volatility charts to vega 5" on our radar (feel free to start looking into that, too).
  • look around the org for any other vega visualizations and do the same two steps on them (remove open in vega editor; open issue about upgrading specs). I think feature-table summarize and composition ancom are two vega vizs, but there might be more. Please let me know if you have any questions!

@thermokarst thermokarst assigned Oddant1 and unassigned thermokarst Jun 1, 2020
@Oddant1 Oddant1 changed the title BUG: Get volatility to display properly in vega again HACK: Remove "Open in Vega Editor" from volatility for now Jun 2, 2020
@Oddant1
Copy link
Member Author

Oddant1 commented Jun 2, 2020

@thermokarst I'm not seeing any other viz's that use vega

Copy link
Contributor

@thermokarst thermokarst left a comment

Choose a reason for hiding this comment

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

This LGTM, but please undo the changes to vega-embed.min.js - thanks!

@Oddant1
Copy link
Member Author

Oddant1 commented Jun 2, 2020

@thermokarst I believe the js changes were also rolled back here, but do you want me to also role back the other changes I made while trying to make the viz work.

@thermokarst
Copy link
Contributor

@thermokarst I believe the js changes were also rolled back here

Sorry, I realize where the caching was coming from - I was navigating to the PR via my notifications. The notification I was clicking on was an old when. Sorry for that!

do you want me to also role back the other changes

Yes please!

@Oddant1 Oddant1 assigned thermokarst and unassigned Oddant1 Jun 3, 2020
@thermokarst thermokarst merged commit 183051b into qiime2:master Jun 3, 2020
@Oddant1 Oddant1 deleted the vega_not_displaying branch June 25, 2020 20:35
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.

2 participants