qiime2 / q2-taxa

BSD 3-Clause "New" or "Revised" License
3 stars 29 forks source link

Fix two bugs related to changing taxonomic level in the barplot #126

Closed fedarko closed 5 years ago

fedarko commented 5 years ago

Previously, if a user changed the bar width and then changed the Taxonomic Level, the bar widths would've gotten reset (due to appendTaxaPicker() calling init() instead of render()). This problem also impacted the currently selected color scheme (aka "Color Palette") -- it would keep getting reset to schemeAccent. (I looked around, and I don't think either of these bugs had open issues.)

Now, init() takes as an argument a state object which stores some information about the current state of the bar plot visualization. I also added helper functions to toolbar.js (getBarWidth() and getColorScheme()) which attempt to get information about parts of this state from the DOM and fall back on sane defaults in the case where these elements don't exist (yet).

As mentioned in the commit messages, this sort of solution could probably be used to address other problems in the visualization (e.g. #87, #90, #91) if this design pattern doesn't seem egregious.

There are definitely still some quirks in the visualization (mostly related to visibility selection stuff), but it should definitely be more pleasant to use the bar plot now :)

Edit: as with prior PRs I've made here, github is showing all of the unique commits on my fork of q2-taxa. The pertinent commits are the two most recent ones (30a17d2 and ef72b55).

fedarko commented 5 years ago

@thermokarst yay! want me to update the changelog again re: the now-fixed bugs?

thermokarst commented 5 years ago

Yes please!

fedarko commented 5 years ago

Ok, done! Thanks a ton!