Fix two bugs related to changing taxonomic level in the barplot #126
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This looks a lot nicer.
Will make it easier to adjust this in the future, if desired.
The min size increase should at least prevent the labels from getting squished together.
TODOs remaining: -Preserve the bar width on changing taxonomic level -When filtering the bar chart to show a certain taxon/level, ensure that this filtering is respected upon changing the bar width.
This is done by passing bar width to init(), which in turn is now done by a "state" object that init() accepts as an argument. It seems like some of the open problems in the bar plot visualization (e.g. qiime2#91) could be addressed by storing more information about the visualization's state. (Not sure just passing this around between functions is the *best* way to structure things, though.)
Previously, changing the taxonomic level would revert the color scheme back to the default (schemeAccent). Now, it should stick with whatever the user had selected. This is done by using the new "state" option of init(). I imagine we could also use this in the future for outstanding issues like qiime2#87.
thermokarst
approved these changes
Jul 26, 2019
|
@thermokarst yay! want me to update the changelog again re: the now-fixed bugs? |
|
Yes please! |
|
Ok, done! Thanks a ton! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, if a user changed the bar width and then changed the
Taxonomic Level, the bar widths would've gotten reset (due toappendTaxaPicker()callinginit()instead ofrender()). This problem also impacted the currently selected color scheme (aka "Color Palette") -- it would keep getting reset toschemeAccent. (I looked around, and I don't think either of these bugs had open issues.)Now,
init()takes as an argument astateobject which stores some information about the current state of the bar plot visualization. I also added helper functions totoolbar.js(getBarWidth()andgetColorScheme()) 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).