fix(heatmap): compute nice legend items from color scale #1273
fix(heatmap): compute nice legend items from color scale #1273markov00 merged 17 commits intoelastic:masterfrom
Conversation
|
Great improvement 🎉 Wondering if a future PR could arrange the belonging color ramps vertically, it'd be easier to distinguish like colors (within shades of blue etc) vertically, than with the large horizontal jumps. Also, I wonder about the greater than vs greater than or equal here, also apparently the same color, is it intended?
Eyeballing some other mock, it's just tests but yellow is something to avoid in production code as a color legend on a white background, right? |
|
Also, again it's just a mock but it's something that people reference: would be great to avoid the inaccessible red/green color pairing, eg. using red/blue or some such |
| function isFilteredValue(filterRanges: Array<[number, number | null]>, value: number) { | ||
| return filterRanges.some(([min, max]) => { | ||
| if (max !== null && value > min && value < max) { | ||
| if (max !== null && value >= min && value < max) { | ||
| return true; | ||
| } | ||
| return max === null && value > min; | ||
| return max === null && value >= min; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Minor: this to me would be an easier read:
function isFilteredValue(filterRanges: Array<[number, number | null]>, value: number) {
return filterRanges.some(([min, max]) => min <= value && value < (max ?? Infinity));
}
Also, should it be named hasFilteredValue or some such?
There was a problem hiding this comment.
The semantics of it suggests that it returns true if at least one value falls within min/max (or min/Infinity), so maybe, hasKeptValue or hasRetainedValue is a better name. Words like keep/retain or eliminate are less ambiguous than filter; I never know if filtering refers to the kept part or the eliminated part 😄
There was a problem hiding this comment.
thanks robert, this looks way batter e500773
There was a problem hiding this comment.
Nice change, thanks! Minor (not worth a change unless you're planning further pushes anyway): visibilityFilterRanges is a bit long and doesn't give away its secret, may it be shorter/clearer? Not 100% sure of the meaning, something like visibleRanges or shownRanges or rangesToShow?
There was a problem hiding this comment.
To clarify, visibilityFilterRanges doesn't tell if what's inside are included or the opposite, eliminated. Unfortunately, the word filter is vague about what's kept and what's shed. It doesn't help that there's some compounding of name ambiguity, eg. what does "deselected" mean in deselectedTicks and deselectedDataSeries, deselected from what?
Maybe eventually these words could be positive, ie. a list of selected/retained ticks rather than exclusions. Also, ON_TOGGLE_SERIES_VISIBILITY (or whatever the meaning) instead of ON_TOGGLE_DESELECT_SERIES; avoidance of a negative, and more meaning than "select"
There was a problem hiding this comment.
You are right Robert, I've revisited the code, and correct the naming 4228efc
I've looked more into the details and the naming was also flipped.
The deselected... is another story and we can chat about that in the future.
rshen91
left a comment
There was a problem hiding this comment.
LGTM tested locally. It took me an embarrassingly long time to realize dedupBands was an abbreviation of deduplicateBands. Not sure if this is something we should change.
| return { | ||
| color, | ||
| label: `> ${spec.valueFormatter ? spec.valueFormatter(tick) : tick}`, | ||
| label: `>${i === 0 ? '=' : ''} ${formattedStart}`, |
There was a problem hiding this comment.
I wonder if we can use a symbol ≥
There was a problem hiding this comment.
The logic has changed a bit and now every item is GTE the value in the legend
4228efc
nickofthyme
left a comment
There was a problem hiding this comment.
LGTM, tested locally.
monfera
left a comment
There was a problem hiding this comment.
LGTM, thanks for the changes! My comments are optional code convention ones (clearer naming) or just mentioning possible edge cases
…lastic-charts into 2021_07_28-heatmap_fix_legend
| seriesIdentifiers: [ | ||
| { | ||
| key: label, | ||
| specId: label, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
[nit] similar to how you did it below with path, this could be a single line, to let more "data" to be seen without scrolling
|
🎉 This PR is included in version 33.2.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
The heatmap legend now correctly shows all the colors and values associated with the chosen scale, without duplicates and with all the available color bands computed.
Details
The previous legend implementation lacks various details: the legend ticks provided by d3 are not always what we want on the legend and the relationship between colors and ticks wasn't perfect. The code now takes in consideration the various scale types characteristics applying the missing values to renders correctly the legend items without duplicates.
With the changes, we also fixed the ability to hide all the cells from the legend as highlighted in the bug #1192
Issues
fix #1166
fix #1191
fix #1192
Checklist