elastic-charts icon indicating copy to clipboard operation
elastic-charts copied to clipboard

feat(tooltip): pinned tooltip with actions

Open nickofthyme opened this issue 2 years ago • 5 comments

Summary

Adds sticky tooltip interaction whenever Tooltip.actions are defined. See demo story here.

Features:

  • Right-click to stick tooltip
  • Select series on stuck tooltip
  • Disables stickiness for empty tooltips
  • trigger stickiness from tooltip actions or flag
  • trigger actions with selected series
  • global click listener to dismiss pinned tooltip
  • Clear pinned tooltip on resize, esc or outside click
  • limit scroll height to 5 items
  • scroll nearest elements into view

Chart types with context menu:

TODOs

  • Configure what is passed back to user (heatmap & flame)
  • Block tooltip render when brushing (xy)

https://user-images.githubusercontent.com/19007109/185769263-d0e4b4c1-ffb4-49e4-9963-0944c74e2cb1.mp4

Testing in kibana

To test against this PR on kibana update the package.json to point to the tooltip-actions-dist branch.

// package.json
{
  "dependencies": {
    "@elastic/charts": "https://gitpkg.now.sh/nickofthyme/elastic-charts/packages/charts?tooltip-actions-dist"
  }
}

Then run bootstrap with --no-validate flag...

yarn kbn bootstrap --no-validate

See kibana example/testing/demo at https://github.com/elastic/kibana/pull/141503

Details

Modifies the interactions redux state to add tooltip stuck/selection state and account for the stuck state and disable existing interactions like drag and click while stuck.

The esc option to cancel the brush interaction was added a while ago but was a little loose when adding and removing the keyboard event listeners.

I have noticed that some of the tooltip interactions are tightly coupled to the redux store/selectors. This might become problematic as we are adding event listeners within packages/charts/src/components/chart_container.tsx that may not be implemented inside the reducers. For example, we had a keyup listener that was added but the reducer that it triggers may not actually do any logic.

We should have a better way to communicate interactions to the component that does not require as much redundant checks or logic.

Scroll into view demo

https://user-images.githubusercontent.com/19007109/187324616-175be9cb-d09c-40e1-bde0-3c2341313983.mp4

This feature works by scrolling to the first highlighted tooltip value. Some improvements on this logic could be...

  • Some type of collapsing on groups for charts with hierarchical structures to always show highlighted values.
  • Collapse values in between until tooltip is pinned.
  • Currently the order of tooltip items is arbitrary but if we change this to be nearest-first we could have a better interaction. Currently, the closest is not element is not always scrolled to view, as seen at the end of the recording above where the cursor is closest to the line point but the bar values is focused.
  • Shuffle the items to allow showing all highlighted values. Might be a bad idea

Checklist

  • [x] The proper chart type label has been added (e.g. :xy, :partition)
  • [x] The proper feature labels have been added (e.g. :interactions, :axis)
  • [ ] The :theme label has been added and the @elastic/eui-design team has been pinged when there are Theme API changes
  • [ ] All related issues have been linked (i.e. closes #123, fixes #123)
  • [ ] New public API exports have been added to packages/charts/src/index.ts
  • [ ] Unit tests have been added or updated to match the most common scenarios
  • [ ] The proper documentation and/or storybook story has been added or updated
  • [ ] The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)
  • [ ] Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

nickofthyme avatar Aug 10 '22 04:08 nickofthyme

Agree with Robert, this only partially is related to the tooltip, this is more a contextual menu than a sticky/pinned tooltip.

For the interaction/cancellation/clearing it should behave exactly as a right-click context menu in the browser: if you click elsewhere or in one item of the menu, the menu will disappear immediately

markov00 avatar Aug 16 '22 15:08 markov00

It seems like if there is a custom tooltip bit, it won't render the "right click to stick" message anymore: Screenshot 2022-09-29 at 18 18 25

It still works though: Screenshot 2022-09-29 at 18 18 29

Maybe I'm using it wrong

flash1293 avatar Sep 29 '22 16:09 flash1293

@flash1293 Nope you are correct, I am working on adding support for the customTooltip option.

nickofthyme avatar Sep 29 '22 17:09 nickofthyme

Not sure whether this is some kind of bug with my way of loading this branch, but the actions are not typed: Screenshot 2022-09-30 at 09 15 45

flash1293 avatar Sep 30 '22 07:09 flash1293

I tested a bit more and the current api is a bit limiting. There can only be one set of actions for a chart no matter what value the user clicked. This is an assumption that holds true most of the time, but not necessarily always.

Also due to this setup it's not possible to only fetch some information if the user is actually opening the tooltip which might be necessary at some point (we could work around it right now, but not forever) - for example we might want to disable an action if a certain saved object is created already, but we need to check with an additional request. We can only send this request if the user actually pinned the tooltip - right now there is no place to do this

What I'm suggesting is instead of a prop which takes a list of actions, having a prop which takes a component to render the actions on its own

e.g. like this: old:

  actions: {[ label: "abc", onSelect: (values) => handleClick(values) ]}

new:

import { TooltipAction } from '@elastic/charts';

function TooltipActions(values) {
  useEffect(() => {
    // some async stuff
  })
  return <>
    <TooltipAction label="abc" onClick={() => handleClick(values)}>
  <>
}

// ...
actions: {TooltipActions}

it will also come in handy later for having more sophisticated interactivity in there instead of just a list of links to click (I remember some mockups with mini-forms in there).

Let me know what you think.

flash1293 avatar Oct 05 '22 07:10 flash1293

Deployments - 912185fdda7649464959c0e9e999ac13d09fe3fb

  • Storybook (link)
  • e2e server (link)
  • Playwright report (link)

elastic-datavis[bot] avatar Oct 05 '22 19:10 elastic-datavis[bot]

@monfera I'm getting an issue where pinning the tooltip then quickly clicking outside of the chart, causes the chart to go blank on the next render and following renders.

Screen Recording 2022-10-11 at 05 10 44 PM

I believe it is somehow related to contextRestoreHandler specifically the code in the setTimeout below. Increasing this timeout makes it easier to trigger the blank chart (i.e. time to pin and blur chart). Any thoughts on why this maybe the case?

https://github.com/elastic/elastic-charts/blob/accb50a790c1785c8b691e66f52587a88611dd66/packages/charts/src/chart_types/flame_chart/flame_chart.tsx#L1192-L1195

nickofthyme avatar Oct 12 '22 00:10 nickofthyme

@monfera I'm getting an issue where pinning the tooltip then quickly clicking outside of the chart, causes the chart to go blank on the next render and following renders.

Hey @nickofthyme this https://github.com/elastic/elastic-charts/pull/1782/commits/98f5436fafacba186c1a582b53f89b1e1816c045 should solve the issue. You where registering a mousemove listener when handling the context menu click. This cause the handleMouseDragMove to be called making the glitch to appear. You can better reproduce the problem by right-click on a node and by dragging outside the context menu.

markov00 avatar Oct 13 '22 14:10 markov00

@gvnmagni These changes are nearly code complete, just fixing up the code checks and doing a final check on screenshots. Please take a look at let me know if there are any changes you'd like to make. I am missing a few colors for the dark mode namely the tooltip hover highlighter color and the action text color. If they are the same that's fine just want to make sure as the figma colors don't match any of the eui color values here.

nickofthyme avatar Oct 24 '22 19:10 nickofthyme

buildkite update vrt

nickofthyme avatar Oct 24 '22 19:10 nickofthyme

Thank you Nick! I believe @markov00 is already working on both colors and will be fixed soon!

gvnmagni avatar Oct 25 '22 10:10 gvnmagni

buildkite update vrt

nickofthyme avatar Nov 01 '22 18:11 nickofthyme

buildkite update vrt

nickofthyme avatar Nov 02 '22 04:11 nickofthyme

Deployments - f4d0656

  • Storybook (link)
  • e2e server (link)
  • Playwright report (link)

The changes on the VRTs from master looks good to me and only related to:

  • the added tooltip border
  • a tooltip rendered with the prompt if the list of values where too long

markov00 avatar Nov 03 '22 16:11 markov00

buildkite update vrt

nickofthyme avatar Nov 04 '22 18:11 nickofthyme

Everything looks good expect for the fact that the control+click on mac doesn't works as expected. It initially pins the tooltip but a subsequent mouse movement will unpin the tooltip. control+click should follow the same behaviour of the secondary click on mac (usually the two finger click)

Good catch, fixed in 0367fed

nickofthyme avatar Nov 07 '22 17:11 nickofthyme