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: show tooltip for external events #698

Merged
merged 19 commits into from
Jun 18, 2020

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Jun 9, 2020

Summary

Adds the ability to show tooltip on a synced chart.

The <Settings /> component has a new prop that allows the consumer to describe the behavior of external events (in this case the behavior of the tooltip if an external event was received from the chart).
The main settings is the ability to turn on the tooltip visibility if an external pointer event is received. Then the consumer can also configure the placement of the tooltip and the boundary, as it can be done for a normal tooltip.

The ExternalPointerEventsSettings is structured like that to accommodate in the future more settings, not only for tooltip but for anything that can be configured in case of external events (like change/update the legend extra value when we receive an event from another chart).

export interface SettingsSpec extends Spec {
...
 /**
   * {@inheritDoc ExternalPointerEventsSettings}
   * @alpha
   */
  externalPointerEvents: ExternalPointerEventsSettings;
...
}

/**
 * The settings for handling external events.
 * @alpha
 */
export interface ExternalPointerEventsSettings {
  /**
   * Tooltip settings used for external events
   */
  tooltip?: {
    /**
     * `true` to show the tooltip when the chart receive an
     * external pointer event, 'false' to hide the tooltip.
     * @defaultValue `false`
     */
    visible?: boolean;
    /**
     *  {@inheritDoc TooltipProps.placement}
     */
    placement?: Placement;
    /**
     *  {@inheritDoc TooltipProps.fallbackPlacements}
     */
    fallbackPlacements?: Placement[];
    /**
     *  {@inheritDoc TooltipProps.boundary}
     */
    boundary?: HTMLElement | 'chart';
  }
}

Jun-10-2020 14-51-27

close #695

Checklist

Delete any items that are not 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

@markov00 markov00 added :interactions Interactions related issue :tooltip Related to hover tooltip wip work in progress enhancement New feature or request labels Jun 9, 2020
@markov00 markov00 force-pushed the 2020_06_09-sync_and_show_tooltips branch from b40c1b2 to fd6b7df Compare June 9, 2020 21:23
@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2020

Codecov Report

Merging #698 into master will increase coverage by 0.35%.
The diff coverage is 86.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #698      +/-   ##
==========================================
+ Coverage   74.80%   75.16%   +0.35%     
==========================================
  Files         265      282      +17     
  Lines        8541     8877     +336     
  Branches     1714     1675      -39     
==========================================
+ Hits         6389     6672     +283     
- Misses       2099     2146      +47     
- Partials       53       59       +6     
Impacted Files Coverage Δ
src/chart_types/goal_chart/state/chart_state.tsx 59.18% <0.00%> (ø)
.../chart_types/partition_chart/state/chart_state.tsx 74.50% <0.00%> (ø)
src/state/chart_state.ts 87.09% <ø> (ø)
...state/selectors/get_internal_is_tooltip_visible.ts 75.00% <50.00%> (ø)
src/components/tooltip/get_tooltip_settings.ts 57.14% <57.14%> (ø)
src/state/selectors/is_external_tooltip_visible.ts 75.00% <75.00%> (ø)
src/components/tooltip/tooltip.tsx 67.50% <88.88%> (+1.26%) ⬆️
...rc/chart_types/xy_chart/renderer/dom/crosshair.tsx 80.39% <100.00%> (+1.66%) ⬆️
..._types/xy_chart/state/selectors/get_cursor_band.ts 88.23% <100.00%> (+1.13%) ⬆️
...hart/state/selectors/get_elements_at_cursor_pos.ts 95.00% <100.00%> (ø)
... and 30 more

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 c540c8a...dc12763. Read the comment docs.

src/specs/settings.tsx Outdated Show resolved Hide resolved
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.

Looking good. Have you noticed the performance lag on the Cursor Update Action story?

@markov00
Copy link
Member Author

Have you noticed the performance lag on the Cursor Update Action

Yes the performance lag happen only when you have the Action tab open, if you switch to Knobs or Source there should be no lag

@markov00 markov00 removed the wip work in progress label Jun 10, 2020
@markov00 markov00 marked this pull request as ready for review June 10, 2020 14:27
@markov00 markov00 requested a review from nickofthyme June 10, 2020 14:37
@nickofthyme
Copy link
Collaborator

I noticed that when hovering on the upper chart, the tooltip below is placed over the highlighted region. In this case I would expect the tooltip on the lower chart to be place at either of the two blue boxes. Wouldn't you agree?

Screen Shot 2020-06-10 at 10 12 09 AM

What's strange is that the opposite is not the same. When hovering on the lower chart, the upper chart displays the tooltip in the expected position.

Screen Shot 2020-06-10 at 10 14 21 AM

@nickofthyme
Copy link
Collaborator

On another note, the vertical placement of the "following" tooltip seems clunky. It just sticks to the top. I think it may be too much to handle in this pr but a future enhancement could be to match the relative (i.e. percentage or ratio of) height from the "active" tooltip.

So if the "active" tooltip is in the middle 50% height, then the "following" would be placed at 50% of its chart height. I think this would look a lot better.

@markov00
Copy link
Member Author

markov00 commented Jun 10, 2020

I noticed that when hovering on the upper chart, the tooltip below is placed over the highlighted region. In this case, I would expect the tooltip on the lower chart to be placed at either of the two blue boxes. Wouldn't you agree?

@nickofthyme this is because we are using the placement: Placement.BottomEnd for the external tooltip placement in that example. Unfortunately, as you are already noticing, the tooltip is anchored to the top of chart and the placement will just move the tooltip around that anchor. We should find a way to change that anchor to something like you suggested: anchor to the top, to the bottom, the center of to the current tooltip value (we should find a smart way when we have multiple data points on the same tooltip, should we place the tooltip in the middle/average point of all the values?)

@nickofthyme
Copy link
Collaborator

@markov00 Oh that makes sense because the anchor element is just a strip across the highlighted region.

Another thing I noticed is the follow tooltip type. I think the follow type should not be allowed for external tooltips cuz it is spatially specific. I also think the ExternalPointerEventsSettings should include a type which would default to the tooltip settings type but allows them to be different.

Screen Recording 2020-06-10 at 10 46 AM

Also, small possibly related issue. I notice an empty tooltip when the "following" tooltip is on the ends of the chart. See black dot show with blue arrows.

Screen Recording 2020-06-10 at 10 50 AM

@markov00
Copy link
Member Author

@markov00 Oh that makes sense because the anchor element is just a strip across the highlighted region.

Another thing I noticed is the follow tooltip type. I think the follow type should not be allowed for external tooltips cuz it is spatially specific. I also think the ExternalPointerEventsSettings should include a type which would default to the tooltip settings type but allows them to be different.

Whit this commit I'm fixing the fact that: the TooltipType used for external events is only the VerticalCursor (no follow, no Crosshair). I've also fixed the rendering of the cursor band if the tooltip type of a chart is defined as None, showing always the cursor band if the events comes from an external event.

The tooltip disappearing bug was also fixed b25e01f

src/specs/constants.ts Outdated Show resolved Hide resolved
src/specs/settings.tsx Outdated Show resolved Hide resolved
@markov00 markov00 requested a review from nickofthyme June 18, 2020 09:58
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.

Changes look good to me. Small black circle tooltip issue is gone.

Only change I would suggest is to add a knob to change the tooltip type for the "active" chart to see that the "following" tooltip will always be vertical or none.

Comment on lines +14 to +15
width: 0;
height: 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -185,11 +180,11 @@ const TooltipPortalComponent = ({ anchor, scope, settings, children, visible, ch
}, [visible, anchorNode, position?.left, position?.top, position?.width, position?.height]);

useEffect(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I Love love love this, I don't know why I made this so overly complicated. I swear I tried this but couldn’t get it to work right. Maybe I was trying to prevent recreating the portal node every time it's hidden.

I also like that the portal now disappears every time.

@markov00 markov00 merged commit cc31739 into elastic:master Jun 18, 2020
@markov00 markov00 deleted the 2020_06_09-sync_and_show_tooltips branch June 18, 2020 14:48
markov00 pushed a commit that referenced this pull request Jun 24, 2020
# [19.6.0](v19.5.2...v19.6.0) (2020-06-24)

### Features

* show tooltip for external events ([#698](#698)) ([cc31739](cc31739)), closes [#695](#695)
@markov00
Copy link
Member Author

🎉 This PR is included in version 19.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Jun 24, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request :interactions Interactions related issue released Issue released publicly :tooltip Related to hover tooltip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tootip sync mechanism
3 participants