search

qiime2 / q2-longitudinal

QIIME 2 plugin for paired sample comparisons
BSD 3-Clause "New" or "Revised" License
9 stars 18 forks source link

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

Closed Oddant1 closed 4 years ago

Oddant1 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

I think we need everything listed below, except width:

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

Oddant1 commented 4 years ago

https://github.com/qiime2/q2-longitudinal/pull/154/commits/211098c66dc03949f5d23993f83f55a98f1df273 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 :joy:

Oddant1 commented 4 years ago

https://github.com/qiime2/q2-longitudinal/pull/154/commits/f02690b2d55cb11dfa2b25aa36ed277b8be4461d Turns out you can just remove the binding to the textbox and it still works just fine.

Oddant1 commented 4 years ago

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

Oddant1 commented 4 years ago

@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 commented 4 years ago

@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!