Add option to adjust gap between tree and first barplot layer #464
Conversation
Now checks to ensure that input value is valid. Also do the main computation and setting in barplot-panel-handler rather than empress for readability.
| @@ -143,6 +144,16 @@ define([ | |||
| // length divided by 10 is :) | |||
| this.borderLength = BarplotLayer.DEFAULT_LENGTH / 10; | |||
|
|
|||
| // Initialize default spacing between tree and first barplot layer | |||
| // as well as change behavior. | |||
| this.borderGap = 10; | |||
There was a problem hiding this comment.
Per our call earlier today, the suggestion is to make this space small by default - just making a note here to have a record.
gibsramen
changed the title
There was a problem hiding this comment.
Thanks so much @gibsramen!!!
Per discussion with Gibs earlier today I modified this slightly in the following ways:
-
Use the same "units" for the initial gap as for barplot lengths, making things a bit more consistent for the user. The default gap value is now 10, which matches the default barplot border length.
-
Store the current gap value only in
BarplotPanelHandler, and haveEmpressretrieve this value when needed rather than keeping track of it inEmpress._displacementFrac. The rationale for this is that it reduces the amount of dataEmpressneeds to keep track of, and matches some of the other ways in which the barplot functionality works (e.g. how the border color / length are also retrieved from theBarplotPanelHandler).
Provided that these changes look good, I think we can merge this in. Sorry for taking so long to get to this.
|
@gibsramen, if this looks good to you let me know and I'll be happy to merge
this to the master branch.
…On (Jan-22-21|15:59), Marcus Fedarko wrote:
@fedarko approved this pull request.
Thanks so much @gibsramen!!!
Per discussion with Gibs earlier today I modified this slightly in the following ways:
1. Use the same "units" for the initial gap as for barplot lengths, making things a bit more consistent for the user. The default gap value is now 10, which matches the default barplot border length.
2. Store the current gap value only in `BarplotPanelHandler`, and have `Empress` retrieve this value when needed rather than keeping track of it in `Empress._displacementFrac`. The rationale for this is that it reduces the amount of data `Empress` needs to keep track of, and matches some of the other ways in which the barplot functionality works (e.g. how the border color / length are also retrieved from the `BarplotPanelHandler`).
Provided that these changes look good, I think we can merge this in. Sorry for taking so long to get to this.
--
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub:
https://urldefense.com/v3/__https://github.com/biocore/empress/pull/464*pullrequestreview-574701478__;Iw!!Mih3wA!Rh9uN3yU9xohsHyhHtjhh9mw8RfT2eRKgJVf7Fp_dw6bg7F9V5gj5-rMWTH0ZX4$
|
|
LGTM :) |
|
Thanks so much both! |
Closes #453
Adds a new element to the side panel that allows users to manually set the distance between the tree and the first barplot layer. Currently works by specifying a percentage increase to
_maxDisplacement.Default value is 10 (1 + 10% = 1.1) to keep with current default behavior.Default value changed to 5 as we decided to decrease the default gap.OLD:
var maxD = 1.1 * this._maxDisplacement;NEW:
var maxD = (1 + this._displacementFrac) * this._maxDisplacement;A new functionEmpress.prototype.changeBorderGapjust divides the input value by 100. This can likely be rewritten to not use a whole function - depends on what people think looks more clean.Current Status
Separation lengthempress.js