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

Configurable barplot borders #386

Merged
merged 17 commits into from
Sep 22, 2020
Merged

Conversation

fedarko
Copy link
Collaborator

@fedarko fedarko commented Sep 22, 2020

Closes #350, and should at least make generating figures for the paper a lot easier.

This also removes some of the extra whitespace in the barplot panel when Draw barplots? is unchecked -- this was due to a <p> tag being empty but not hidden, which caused some extra vertical space to be taken up.

borders

there was some extra v-space when barplots were available due to
layout but 'draw barplots' was unchecked; this was because a <p>
was not hidden (although the one button inside it was). This
discrepancy has since been fixed.
makes things look less cluttered. had too many <hr />s previously
more would be nice but probs ok for now
@emperor-helper
Copy link

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

Copy link
Member

@ElDeveloper ElDeveloper left a comment

Choose a reason for hiding this comment

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

Functionality looks great, and is going to be very useful overall. One unexpected thing I noticed is that adding border around barplots adds space between the tips and the first layer. Technically the approach is valid but I think what we've noticed that would be useful for figures is the space between barplot layers, not space between barplots and the tree.

empress/support_files/js/empress.js Outdated Show resolved Hide resolved
empress/support_files/js/empress.js Show resolved Hide resolved
tests/test-barplots.js Show resolved Hide resolved
@fedarko
Copy link
Collaborator Author

fedarko commented Sep 22, 2020

One unexpected thing I noticed is that adding border around barplots adds space between the tips and the first layer. Technically the approach is valid but I think what we've noticed that would be useful for figures is the space between barplot layers, not space between barplots and the tree.

Yeah, this is because the code is technically allocating space for a border to be drawn here. It looks a bit clearer if you select black as the border color:

bord

I would kind of prefer to keep it this way, just because I think consistently drawing borders this way is more intuitive (at least to me). However I'd be down to (probably in a new PR) add in an option to toggle whether or not to draw borders before the innermost and after the outermost barplot layers.

@fedarko
Copy link
Collaborator Author

fedarko commented Sep 22, 2020

I think changes should be good to merge in whenever, pending @ElDeveloper's approval of the two open comments. thanks!

@ElDeveloper
Copy link
Member

Looks great, thanks both!

@ElDeveloper ElDeveloper merged commit b22fad2 into biocore:master Sep 22, 2020
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.

Add barplot padding/border options
3 participants