feat(tooltip): add custom headerFormatter

[emmacunningham]

Summary

addresses #197 & #191

This PR introduces a SettingsSpec prop headerFormatter which the user can use to define a custom header formatter for the tooltip headers.

headerFormatter has the type (data: TooltipValue) => JSX.Element | string so that a user can either specify a simple value formatting function or a more complex one should they need to return a JSX element to be able to add additional text and/or styling that is not provided by the internal chart styles.

We use this feature in Discover charts to show a custom message for the first and last bars in a series when they contain partial buckets:

Original Discover chart:

Version using elastic-charts with custom headerFormatter:

Notable implementation details:

• emptyFormatter was changed from always returning a string to returning the raw value; this was necessary because in Discover, we do a check to see if a value warrants the special header or not
• if headerFormatter is defined, emptyFormatter is used in place of the tick formatter from the relevant domain axis. This means that if headerFormatter is defined, the user must explicitly format the values themselves (in the Discover implementation, we re-use the axis formatter if the condition to display the special header is not met)
• in the tooltip rendering component <p> was changed to <div> to allow for nested elements (Kibana's DOM nesting validator throws a warning for nested elements in a <p>)
• BREAKING CHANGE AND REFACTOR With the addition of the header formatter prop, SettingsSpec would take three tooltip-related props: tooltipType, tooltipSnap, and tooltipHeaderFormatter. Instead of three different props all with the tooltip namespace, in this PR we create an interface for tooltip-related configuration to simplify things. This is a breaking change, but as far as I can tell, no one is using tooltipType or tooltipSnap right now, so it shouldn't impact anyone to change this now. Now, we define TooltipProps and in place of three individual props, expose tooltipProps:

interface TooltipProps {
  type?: TooltipType;
  snap?: boolean;
  headerForrmatter?: TooltipValueFormatter;
}

interface SettingSpecProps {
  chartStore?: ChartStore;
  theme?: Theme;
  rendering: Rendering;
  rotation: Rotation;
  animateData: boolean;
  showLegend: boolean;
  /** Props for customizing the chart tooltip on hover */
  tooltip?: TooltipType | TooltipProps;
  /** ... etc remaining props */
}

See discussion below for more on the new tooltip prop on SettingSpecProps.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

• Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
• This was checked for cross-browser compatibility, including a check against IE11
• 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
• Each commit follows the convention

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@d7e1960). Click here to learn what that means.
The diff coverage is 78.57%.

@@            Coverage Diff            @@
##             master     #233   +/-   ##
=========================================
  Coverage          ?   97.73%           
=========================================
  Files             ?       36           
  Lines             ?     2654           
  Branches          ?      605           
=========================================
  Hits              ?     2594           
  Misses            ?       52           
  Partials          ?        8

Impacted Files	Coverage Δ
src/lib/utils/interactions.ts	100% <ø> (ø)
src/state/chart_state.ts	96.85% <100%> (ø)
src/lib/series/tooltip.ts	100% <100%> (ø)
src/specs/settings.tsx	95.94% <72.72%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7e1960...3efe360. Read the comment docs.

[nickofthyme]

Run storybook locally and verified feature. 👍

Only one suggestion in tests.

[nickofthyme]

👍

[nickofthyme]

This seems like it should be a test in a describe rather than in a comment. Then all the setup could be in a beforeEach to run both assertions you are making below the commented lines?

describe('can use a custom tooltip header formatter', () => {
  beforeEach(() => {
    const axisSpec: AxisSpec = {
      id: AXIS_ID,
      groupId: spec.groupId,
      hide: true,
      showOverlappingTicks: false,
      showOverlappingLabels: false,
      position: Position.Bottom,
      tickSize: 30,
      tickPadding: 10,
      tickFormat: (value: any) => `foo ${value}`,
    };
  
    store.addAxisSpec(axisSpec);
    store.addSeriesSpec(spec);
    store.tooltipType.set(TooltipType.Crosshairs);
    store.tooltipHeaderFormatter = undefined;
    store.computeChart();
  });

  test('with no tooltipHeaderFormatter defined, should return value formatted using xAxis tickFormatter', () => {
    store.setCursorPosition(10, 10);
    expect(store.tooltipData[0].value).toBe('foo 1');
  });

  test('with tooltipHeaderFormatter defined, should return value formatted', () => {
    store.tooltipHeaderFormatter = (value: TooltipValue) => `${value}`;
    store.setCursorPosition(10, 10);
    expect(store.tooltipData[0].value).toBe(1);
  });
});

Just a thought, maybe there are other implications to this test I don't see/know.

[emmacunningham]

👍 Good catch and great suggestion!

[markov00]

I've just take a look at the description and the code and I've few comments on that change:

• what do you think of change the tooltip props name from tooltipProps to only tooltip? sounds much more simple and clean
• what if the tooltip prop accept two types: the TooltipType and the TooltipProps? this can simplify a bit the code for consumers that want only to specify the type of tooltip.
• What are the use case where we want to change the full header (x value included) and what are the cases where we just want to add an additional test to the tooltip? I'm asking this because I just want to avoid consumers to go and implement their own header + add their own data formatter on the header if they want just to add a small text for specific cases.
• In the long run we can have the needs to:
  • enable custom header text (as in this PR)
  • enable custom X value formatter
  • enable custom Y values formatter
  • enable general tooltip formatter (all values + header)
    what is your ideas behind that future?

[emmacunningham]

@markov00 replies below inline:

I've just take a look at the description and the code and I've few comments on that change:

* what do you think of change the tooltip props name from `tooltipProps` to only `tooltip`? sounds much more simple and clean

* what if the `tooltip` prop accept two types: the `TooltipType` and the `TooltipProps`? this can simplify a bit the code for consumers that want only to specify the type of tooltip.

Changed the name of tooltipProps on the Settings component to tooltip and allowed this prop to be either TooltipType or TooltipProps; added type guards to check which type the defined prop is and then using the guards to conditionally update depending on the type. See commit 3efe360

* What are the use case where we want to change the full header (x value included) and what are the cases where we just want to add an additional test to the tooltip? I'm asking this because I just want to avoid consumers to go and implement their own header + add their own data formatter on the header if they want just to add a small text for specific cases.

The type annotation for the headerFormatter is (data: TooltipValue) => JSX.Element | string so this allows the user to configure a formatting function that just returns a string if they do not need to further style the inner component. If they just want to format the string, they have the ability to do that and do not need to provide their own implementation of the header.

Another possibility could be to split up these configurations so that TooltipProps looks something like:

interface TooltipProps {
  type?: TooltipType;
  snap?: boolean;
  headerFormatter?: (dataValue: TooltipValue) => string;
  headerElement?: (dataValue: TooltipValue) => JSX.Element;
}

(We need the element to have access to the raw data value or else we won't be able to do conditional displays if they come in as formatted strings.)

In this case, then the user can:

• specify just the headerFormatter without headerElement, which would just render the string formatted per the defined function
• specify just the headerElement which may also include a custom formatter for the data value and allows them to customize the layout within the header
• specify both headerFormatter and headerElement, in which case, we could use the headerFormatter within the headerElement to format the values and control how the formatted value is rendered.

* In the long run we can have the needs to:
  
  * enable custom header text (as in this PR)
  * enable custom X value formatter
  * enable custom Y values formatter
  * enable general tooltip formatter (all values + header)
    what is your ideas behind that future?

I focussed just on the tooltipHeaderFormatter here and created the more abstracted tooltip prop on Settings because the idea is that once we do implement these additional customization features, headerFormatter can get pulled into the larger strategy. Eventually, we may have a tooltip interface that looks more like:

interface TooltipProps {
  type?: TooltipType;
  snap?: boolean;
  headerFormatter?: (xValue: TooltipValue) => JSX.Element | string; // or the broken up version above
  rowFormatter?: (yValue: TooltipValue) => JSX.Element | string; // or the broken up version above
  containerElement?: (header, yValues) => JSX.Element; 
};

Where, if containerElement is defined, the user is defining their own component for rendering the entire tooltip; we give them all of the values they need and they can decide how to structure & style. If they define headerFormatter and/or rowFormatter, then we'd use those formatting functions within containerElement otherwise, we use the default formatted string.

I think the implementation of the header customization doesn't commit us or block us from being able to address these other customizations, but also doesn't require that we tackle the entire question of what and how much the user should be able to customize these other things in order to get this one feature in which we know is needed for both TSVB and Discover replacements.

[markov00]

@emmacunningham cool thanks for the explanation.
So for me it's good to merge. Just remember the breaking change footer :)

[markov00]

🎉 This PR is included in version 6.0.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀