feat(xy): add null bars to geometry indexing #1226

markov00 · 2021-06-29T15:18:52Z

Summary

The showNullValues prop is now available in the Settings.tooltip to allow displaying null Y values in the tooltip.
This should goes hand in hand with the right tooltip formatter to catch and correctly format these null values.

Details

This PR replace: #1206 reducing the scope to only show null values, keeping the current typings intact.

The PR consist of removing the null values checks in the geometry generation phase, moving every geometry (visible or not) within the geometry index map (the map used by the tooltip to detect the cursor-geometry collisions). The geometries with null values are not pushed within the arrays of renderable geometries.

Issues

close #950

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
Any consumer-facing exports were added to packages/charts/src/index.ts (and stories only import from ../src except for test data & storybook)
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

…ull_values_on_tooltip

monfera · 2021-07-12T17:58:09Z

packages/charts/src/chart_types/xy_chart/rendering/bars.ts

+    const absMinHeight = Math.abs(minBarHeight);
+
+    // safeguard against null y values
+    let height = isNil(y0Scaled) || isNil(y) ? 0 : y0Scaled - y;


Minor: this could be substituted with height = y0Scaled - y || 0; using isNil doesn't add a lot of documentational value as the types already reveal they can be undefined or null. Still, it's a subjective call.

Also, as the row below sets y to zero if either is nil (in row 86) maybe reordering helps simplify a bit (eg. setting y and y0Scaled to 0 before; in which case the subtraction will result zero)

I can't replace it with height = y0Scaled - y || 0 because y0Scaled and y can be null and TS rise a warning/error here

monfera · 2021-07-12T18:03:28Z

packages/charts/src/chart_types/xy_chart/rendering/bars.ts

@@ -125,7 +124,7 @@ export function renderBars(
    const width = clamp(seriesStyle.rect.widthPixel ?? xScale.bandwidth, minPixelWidth, maxPixelWidth);
    const x = xScaled + xScale.bandwidth * orderIndex + xScale.bandwidth / 2 - width / 2;

-    const originalY1Value = stackMode === StackMode.Percentage ? y1 - (y0 ?? 0) : initialY1;
+    const originalY1Value = stackMode === StackMode.Percentage ? (isNil(y1) ? null : y1 - (y0 ?? 0)) : initialY1;


May be tangential to this PR, but GeometryValue has x and y as any, and maybe it could be tightened up. If it's not possible to do it globally, it could start by perhaps mapping to NaN instead of null so that eventually, y in GeometryValue is always a number

Yes agree, we can push this into a separate PR I think

monfera · 2021-07-12T18:38:11Z

packages/charts/src/chart_types/xy_chart/rendering/bars.ts

+    if (y1 !== null && initialY1 !== null && filled?.y1 === undefined) {
+      barGeometries.push(barGeometry);
+    }


Should indexedGeometryMap.set(barGeometry) still be called unconditionally? If that were conditionalized, this predicate could go to the top of the forEach.

Btw. this function is almost 200 lines long with a bunch of let and co-dependent mutations and non-obvious type narrowings to number, and I don't comprehend it enough to do a good review, would be great to refactor before, or as part of feature work otherwise it'll be more and more complicated

indexedGeometryMap.set(barGeometry) needs to be called unconditionally to take the advantage of the current picking logic.
Definitely a refactoring here is required!

nickofthyme

Changes LGTM. This is a really clean solution! 🎉

nickofthyme · 2021-07-12T18:38:38Z

packages/charts/src/specs/settings.tsx

+export function getShowNullValues(settings: SettingsSpec): TooltipProps['showNullValues'] {
+  const { tooltip } = settings;


nit

Suggested change

export function getShowNullValues(settings: SettingsSpec): TooltipProps['showNullValues'] {

const { tooltip } = settings;

export function getShowNullValues({ tooltip }: SettingsSpec): TooltipProps['showNullValues'] {

stories/interactions/18_null_values.tsx

stories/interactions/interactions.stories.tsx

monfera · 2021-07-12T19:00:00Z

packages/charts/src/chart_types/xy_chart/rendering/bars.ts

    }

-    const absMinHeight = Math.abs(minBarHeight);
-    let height = y0Scaled - y;
    if (absMinHeight !== undefined && height !== 0 && Math.abs(height) < absMinHeight) {


absMinHeight is always a number, so this part of the condition can be removed.

Also, using a sign rather than retaining the if(height < 0) means that the height !== 0 check is easy to make redundant:

const heightDelta = absMinHeight - Math.abs(height); if (heightDelta > 0) { const heightSign = Math.sign(height); height = heightSign * absMinHeight; y -= heightSign * heightDelta; }

monfera · 2021-07-12T19:02:54Z

packages/charts/src/chart_types/xy_chart/rendering/bars.ts

@@ -81,12 +77,15 @@ export function renderBars(
      y0Scaled = y0 === null ? yScale.scale(0) : yScale.scale(y0);


The let / if imperative construct for y0Scaled may be replaced with a single assignment:

// prettier-ignore const y0Scaled = yScale.type === ScaleType.Log ? (y0 ?? 0) > 0 ? yScale.scale(y0) : yScale.range[yScale.isInverted ? 1 : 0] : yScale.scale(y0 || 0);

It's not super pretty, there may be ways to more deeply simplify though

monfera

LGTM, my comments are only about how the render function could be simplified or at least made easier to understand

markov00 · 2021-07-13T14:10:55Z

LGTM, my comments are only about how the render function could be simplified or at least made easier to understand

Thanks @monfera I will consider the simplification in a subsequent PR

# [33.0.0](v32.0.0...v33.0.0) (2021-07-14) ### Features * **xy:** add null bars to geometry indexing ([#1226](#1226)) ([20b81a9](20b81a9)), closes [#950](#950) * **xy:** hide labels that protrude the bar geometry ([#1233](#1233)) ([be1fb3d](be1fb3d)), closes [#1234](#1234) ### BREAKING CHANGES * **xy:** an API change is introduced: `hideClippedValue` is removed in favor of `overflowConstraints?: Array<LabelOverflowConstraint>;`. The array can contain one or multiple overflow constraints enumerated as `LabelOverflowConstraint`

nickofthyme · 2021-07-14T15:08:07Z

🎉 This PR is included in version 33.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

markov00 added 4 commits June 29, 2021 17:18

refactor: add null bars to geometry indexing

565e28c

Merge branch 'master' into 2021_06_29-show_null_values_on_tooltip

1093bdb

feat: add settings prop to enable null tooltips

62abfee

Merge remote-tracking branch 'upstream/master' into 2021_06_29-show_n…

1223f3c

…ull_values_on_tooltip

markov00 added :interactions Interactions related issue :tooltip Related to hover tooltip :xy Bar/Line/Area chart related labels Jul 6, 2021

markov00 added 3 commits July 6, 2021 12:43

docs: update exposed API

07fa4c1

test: reflect changes in tests

e2df2bd

Merge branch 'master' into 2021_06_29-show_null_values_on_tooltip

d96bd25

markov00 marked this pull request as ready for review July 6, 2021 12:49

markov00 requested a review from nickofthyme July 6, 2021 12:49

markov00 changed the title ~~refactor: add null bars to geometry indexing~~ feat(xy): add null bars to geometry indexing Jul 6, 2021

monfera reviewed Jul 12, 2021

View reviewed changes

nickofthyme approved these changes Jul 12, 2021

View reviewed changes

nickofthyme mentioned this pull request Jul 12, 2021

feat: expose NaN values to tooltips #1206

Closed

7 tasks

monfera reviewed Jul 12, 2021

View reviewed changes

monfera self-requested a review July 12, 2021 21:38

monfera approved these changes Jul 12, 2021

View reviewed changes

markov00 added 3 commits July 13, 2021 16:10

Merge branch 'master' into 2021_06_29-show_null_values_on_tooltip

f455895

fix: correct header

ee09d9b

Merge branch 'master' into 2021_06_29-show_null_values_on_tooltip

9603b75

markov00 merged commit 20b81a9 into elastic:master Jul 14, 2021

markov00 deleted the 2021_06_29-show_null_values_on_tooltip branch July 14, 2021 14:54

nickofthyme added the released Issue released publicly label Jul 14, 2021

markov00 mentioned this pull request Oct 1, 2021

fix(xy): remove wrongly represented null/missing values in tooltip #1415

Merged

5 tasks

markov00 mentioned this pull request Mar 14, 2022

[APM] Show chart tooltip on hover even for empty buckets elastic/kibana#85793

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(xy): add null bars to geometry indexing #1226

feat(xy): add null bars to geometry indexing #1226

markov00 commented Jun 29, 2021 •

edited

Loading

monfera Jul 12, 2021

markov00 Jul 14, 2021

monfera Jul 12, 2021

markov00 Jul 13, 2021

monfera Jul 12, 2021

markov00 Jul 13, 2021

nickofthyme left a comment

nickofthyme Jul 12, 2021

monfera Jul 12, 2021

monfera Jul 12, 2021 •

edited

Loading

monfera left a comment

markov00 commented Jul 13, 2021

nickofthyme commented Jul 14, 2021

		export function getShowNullValues(settings: SettingsSpec): TooltipProps['showNullValues'] {
		const { tooltip } = settings;

	export function getShowNullValues(settings: SettingsSpec): TooltipProps['showNullValues'] {
	const { tooltip } = settings;
	export function getShowNullValues({ tooltip }: SettingsSpec): TooltipProps['showNullValues'] {

		@@ -81,12 +77,15 @@ export function renderBars(
		y0Scaled = y0 === null ? yScale.scale(0) : yScale.scale(y0);

feat(xy): add null bars to geometry indexing #1226

feat(xy): add null bars to geometry indexing #1226

Conversation

markov00 commented Jun 29, 2021 • edited Loading

Summary

Details

Issues

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickofthyme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monfera Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

monfera left a comment

Choose a reason for hiding this comment

markov00 commented Jul 13, 2021

nickofthyme commented Jul 14, 2021

markov00 commented Jun 29, 2021 •

edited

Loading

monfera Jul 12, 2021 •

edited

Loading