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.

Sign up for GitHub

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

Jump to bottom

fix(heatmap): compute nice legend items from color scale #1273

Merged
merged 17 commits into from Aug 16, 2021
Merged

fix(heatmap): compute nice legend items from color scale #1273

merged 17 commits into from Aug 16, 2021

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Jul 28, 2021 •
edited

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

  • The proper chart type label was added (e.g. :xy, :partition) if the PR involves a specific chart type
  • The proper feature label was added (e.g. :interactions, :axis) if the PR involves a specific chart feature
  • Whenever possible, please check if the closing issue is connected to a running GH project
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios

@markov00 markov00 added bug Something isn't working :legend Legend related issue :heatmap Heatmap/Swimlane chart related issue labels Jul 28, 2021
@markov00 markov00 changed the title fix(heatmap): correct legend items fix(heatmap): compute nice ticks from color scale Jul 28, 2021
@markov00 markov00 changed the title fix(heatmap): compute nice ticks from color scale fix(heatmap): compute nice legend items from color scale Jul 28, 2021
@markov00 markov00 marked this pull request as ready for review July 30, 2021 14:24
@monfera
Copy link
Contributor

monfera commented Jul 30, 2021

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?

image

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?

@monfera
Copy link
Contributor

monfera commented Jul 30, 2021

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

monfera
monfera reviewed Aug 2, 2021
View reviewed changes
packages/charts/src/chart_types/heatmap/layout/viewmodel/viewmodel.ts Outdated
Comment on lines 394 to 401
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;
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks robert, this looks way batter e500773

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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"

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@markov00 markov00 requested a review from monfera August 11, 2021 11:57
rshen91
rshen91 approved these changes Aug 11, 2021
View reviewed changes
Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

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.

darnautov
darnautov approved these changes Aug 11, 2021
View reviewed changes
Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

packages/charts/src/chart_types/heatmap/state/selectors/compute_legend.ts Outdated
};

return {
color,
label: `> ${spec.valueFormatter ? spec.valueFormatter(tick) : tick}`,
label: `>${i === 0 ? '=' : ''} ${formattedStart}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can use a symbol ≥

Copy link
Member Author

Choose a reason for hiding this comment

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

done in f19f14b

Copy link
Member Author

Choose a reason for hiding this comment

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

The logic has changed a bit and now every item is GTE the value in the legend
4228efc

packages/charts/src/chart_types/heatmap/state/selectors/get_color_scale.ts Outdated Show resolved Hide resolved
nickofthyme
nickofthyme approved these changes Aug 11, 2021
View reviewed changes
Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally.

monfera
monfera approved these changes Aug 16, 2021
View reviewed changes
Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes! My comments are optional code convention ones (clearer naming) or just mentioning possible edge cases

monfera
monfera reviewed Aug 16, 2021
View reviewed changes
packages/charts/src/chart_types/heatmap/state/selectors/compute_legend.ts Outdated
Comment on lines 27 to 32
seriesIdentifiers: [
{
key: label,
specId: label,
},
],
Copy link
Contributor

Choose a reason for hiding this comment

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

[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

Copy link
Member Author

Choose a reason for hiding this comment

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

done here 783d463

@markov00 markov00 enabled auto-merge (squash) August 16, 2021 14:29
@markov00 markov00 merged commit 0d392ae into elastic:master Aug 16, 2021
@monfera monfera mentioned this pull request Aug 16, 2021
Open
19 tasks
nickofthyme pushed a commit that referenced this pull request Aug 16, 2021
@semantic-release-bot
chore(release): 33.2.3 [skip ci]
640b737 
## [33.2.3](v33.2.2...v33.2.3) (2021-08-16)

### Bug Fixes

* **heatmap:** compute nice legend items from color scale  ([#1273](#1273)) ([0d392ae](0d392ae)), closes [#1166](#1166) [#1191](#1191) [#1192](#1192)
@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 33.2.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monfera monfera monfera approved these changes

@darnautov darnautov darnautov approved these changes

@nickofthyme nickofthyme nickofthyme approved these changes

@rshen91 rshen91 rshen91 approved these changes

Assignees
No one assigned
Labels
bug Something isn't working :heatmap Heatmap/Swimlane chart related issue :legend Legend related issue released Issue released publicly
Projects
None yet
Milestone
No milestone
5 participants
@markov00 @monfera @nickofthyme @darnautov @rshen91