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.

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

feat(tooltip): pinned tooltip with actions #1782

Merged
merged 114 commits into from
Nov 8, 2022

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Aug 10, 2022

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 (moved to #1859)

  • Configure what is passed back to user (heatmap & flame)
  • Block tooltip render when brushing (xy)
  • Define if we need a different prompt if no actions are available but you can pin the tooltip to show all series.
Screen.Recording.2022-08-20.at.04.24.33.PM.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 elastic/kibana#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

Screen.Recording.2022-08-29.at.05.56.53.PM.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

Issues

fix #1857

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • 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 nickofthyme added enhancement New feature or request :interactions Interactions related issue :styling Styling related issue :tooltip Related to hover tooltip labels Aug 10, 2022
@nickofthyme nickofthyme changed the title feat(tooltip): initial sticky interactions feat(tooltip): sticky action interactions Aug 10, 2022
@nickofthyme nickofthyme changed the title feat(tooltip): sticky action interactions feat(tooltip): sticky actions Aug 10, 2022
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.

Nice work! It's a WIP so all these below may be on your list already:

There are some statefulness issues, for example:

  • activating the tooltip, then, as the next step, trying to activate it elsewhere just clears the pinned tooltip
  • even after clearing (right-click on another bar) it's not possible to pin the tooltip, it requires that the mouse first be moved, probably to populate x/y values (right-click DOM interactions also pass on coordinates, so I think there should be no requirement for a mousemove)
  • activating the tooltip, then left-clicking on another bar (or pretty much anything else, except, apparently, the Esc key or another right-click tooltip activation attempt) doesn't clear the pinned tooltip

It'd feel odd to maintain the pinned status if the user clicks (eg. left-clicks) elsewhere. A click is a strong expression of intent, whether activating something else or cancelling a current modality. Our interaction design for this should be such that it's super easy to get out of the pinned mode. So, starting with "literally everything gets the user out of pinned mode" then remove hover from that list (I think only a subsequent hover should leave the tooltip pinned)

Wondering if the pinned tooltip needs the common ☒ button on the top of the tooltip as another means for cancellation. While also observing Fitts law (ie. the unpin hotspot can/should be larger than the visible ☒ or pin symbol)

(Maybe it's done already) It'd also be great to link to an interaction design document. The current way doesn't just pin the tooltip, it in effect establishes a strong, new modality that overrides everything. Worth comparing it to the more fleeting persistence of such tooltips in Tableau. In any case, it's worth making interaction design choices explicit, and supported with comparisons and arguments

Minor: I'd also rename the PR, because it's more of a pinned thing than a sticky one, and whether the "tooltip" has actions or not is secondary, so I'd just call it "pinned tooltip" or similar. "Sticky" also has the issue that its past participle is "stuck" and that has other connotations (unwanted behavior, like a stuck button)

@markov00
Copy link
Member

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

@nickofthyme nickofthyme changed the title feat(tooltip): sticky actions feat(tooltip): pinned tooltip with actions Aug 16, 2022
@nickofthyme nickofthyme marked this pull request as ready for review August 20, 2022 23:26
- configurable max height on tooltip body
- configurable tooltip action prompts from spec
- add disabled option for action config
- cleanup action styles
@nickofthyme
Copy link
Collaborator Author

buildkite update vrt

@nickofthyme nickofthyme enabled auto-merge (squash) November 4, 2022 19:04
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

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)

@nickofthyme
Copy link
Collaborator Author

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

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Everything looks good to me, amazing work Nick!

@markov00 markov00 requested review from monfera and removed request for monfera November 8, 2022 10:25
@nickofthyme nickofthyme requested review from monfera and removed request for monfera November 8, 2022 16:16
@nickofthyme nickofthyme dismissed monfera’s stale review November 8, 2022 16:18

All issues have been addressed

@nickofthyme nickofthyme merged this pull request into elastic:master Nov 8, 2022
@nickofthyme nickofthyme deleted the tooltip-actions branch November 8, 2022 16:18
nickofthyme added a commit that referenced this pull request Nov 8, 2022
BREAKING CHANGE: tooltip actions, interaction behavior and styles

Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>
Co-authored-by: elastic-datavis[bot] <98618603+elastic-datavis[bot]@users.noreply.github.com>
nickofthyme pushed a commit that referenced this pull request Nov 8, 2022
# [51.0.0](v50.2.0...v51.0.0) (2022-11-08)

### Features

* **tooltip:** pinned tooltip with actions ([#1782](#1782)) ([7a054f7](7a054f7))

### BREAKING CHANGES

* **tooltip:** tooltip actions, interaction behavior and styles

Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>
Co-authored-by: elastic-datavis[bot] <98618603+elastic-datavis[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request :flamegraph Flame/Icicle chart related issues :heatmap Heatmap/Swimlane chart related issue :interactions Interactions related issue :partition Partition/PieChart/Donut/Sunburst/Treemap chart related :styling Styling related issue :tooltip Related to hover tooltip :xy Bar/Line/Area chart related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cursor remains visible while brushing [Bar chart] tooltips longer than the page height cause page to jump
5 participants