Histogram widget #170
Histogram widget #170
Conversation
no updating yet
also public api for set / get selection
this fixes the selection growing and shrinking
| --histogram-widget--background-color: #{$color-ui-01}; | ||
|
|
||
| display: block; | ||
| background: var(--histogram-widget--background-color, $color-ui-01); |
There was a problem hiding this comment.
I think this is not working
| background: var(--histogram-widget--background-color, $color-ui-01); | ||
|
|
||
| .as-histogram-widget__header { | ||
| font: #{$font-subheader}; |
There was a problem hiding this comment.
Do we need the curly brackets here?
|
|
||
| .as-histogram-widget__tooltip { | ||
| // ATT REVIEWER: do we want to do this? | ||
| @extend .as-tooltip; |
There was a problem hiding this comment.
I can't think of a better approach to this, so I'd keep it
| */ | ||
| @Method() | ||
| public setSelection(values: number[] | null) { | ||
| this._setSelection(values === null ? values : this._adjustSelection(values)); |
There was a problem hiding this comment.
I think this is hard to read, can we improve it somehow?
| this._setSelection(d1); | ||
| } | ||
|
|
||
| private _setSelection(selection: number[]) { |
There was a problem hiding this comment.
This method is (AFAIK) always called with _adjustSelection can we call _adjustSelection inside this method instead of doing it multiple times outside of it?
| } | ||
|
|
||
| .as-histogram-widget__description { | ||
| font: #{$font-body}; |
There was a problem hiding this comment.
Same, do we need the curly brackets?
| .call(this.brush); | ||
|
|
||
| this.brushArea.on('mousemove', () => { | ||
| const evt = d3event as MouseEvent; |
There was a problem hiding this comment.
Can we avoid using abbreviations in variable names? Same for the bb below
There was a problem hiding this comment.
event would shadow window.event
boundingbox instead of bb makes everything too big imho, and it's just some geometry check
| const node = nodes[i] as Element; | ||
| const bb = node.getBoundingClientRect(); | ||
|
|
||
| if (bb.left <= clientX && |
There was a problem hiding this comment.
Can we extract this conditional to a variable to give it some context? otherwise it's hard to know what we're checking
| } | ||
|
|
||
| // I don't know why this happens | ||
| if (!evt.sourceEvent) { |
There was a problem hiding this comment.
Can we merge this conditional and the one below?
| return false; | ||
| }); | ||
|
|
||
| // True if any of the selection values is inside the data |
There was a problem hiding this comment.
Is this more readable?
this.data.any(value => selectionValue >= value.start && selectionValue <= value.end);
Closes #91
This PR adds a histogram widget to airship
The only thing that I didn't implement is the color range. There's still time to copy the builder approach, though.