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

[BUGFIX] Line highlight on hover refactor using ECharts dispatch #1112

Merged
merged 25 commits into from
May 17, 2023

Conversation

sjcobb
Copy link
Member

@sjcobb sjcobb commented Apr 17, 2023

TBD - need to fix flicker issue due to limitation described in: apache/echarts#18495
UPDATE: Flicker issue is fixed now using Julie's axisPointer.triggerEmphasis approach, this PR is ready for review!

Overview

Early on in the Perses project, we decided to build a custom tooltip with React so that we could have full control. This allows us to only show nearby series in the tooltip, better performance with 1000+ series, and ability to easily format the tooltip differently depending on the number of focused series. Since we went down this path, that also means we need to build custom highlighting behavior since the number of focused series can vary depending on what is set in our yBuffer param in the getNearbySeries function.

This PR improves the implementation by using EChart's dispatch API so we can trigger custom highlight and downplay actions depending on seriesIndex conditions and which series are currently in the currentFocusedData array.

Now hover styles are applied correctly so that only the focused series showing in the tooltip are emphasized!

The blur workaround in #1107 is also no longer needed.

Screen Recordings

Loom demo showing changes and ECharts dispatch API docs

Local example 1
https://user-images.githubusercontent.com/9369625/232504585-8ee1b0fd-41da-4f00-8238-4d4435668c71.mp4

Local example 2
https://user-images.githubusercontent.com/9369625/232507723-7a4c1088-2283-49ae-8cac-7b6fe33c3755.mp4

Checklist

  • Pull request has a descriptive title and context useful to a reviewer.
  • Pull request title follows the [<catalog_entry>] <commit message> naming convention using one of the following catalog_entry values: FEATURE, ENHANCEMENT, BUGFIX, BREAKINGCHANGE, IGNORE.
  • All commits have DCO signoffs.

UI Changes

  • Changes that impact the UI include screenshots and/or screencasts of the relevant changes.
  • Code follows the UI guidelines.

@sjcobb sjcobb marked this pull request as ready for review April 17, 2023 14:05
@sjcobb sjcobb requested review from juliepagano, a team and eunicorn April 17, 2023 14:23
@juliepagano
Copy link
Contributor

It's sort of separate from your change here, but I find our current approach to be kind of confusing in some cases like this one where you get this tooltip flicker of some items that occasionally have peaks near where you're moving the mouse left/right. I wonder if we need to have some chats with UX about the ideal behavior for tooltips/highlights/etc., so we can identify an ideal behavior for tooltip/emphasis/etc. we want to work towards.

Screen.Recording.2023-04-17.at.10.56.50.AM.mov

@juliepagano
Copy link
Contributor

Separate from my previous note, the lines seem extra flicker-y with this change. My guess is that the highlight & downplay events you're triggering are still fighting with the highlight/trigger events from the axis pointer.

Screen.Recording.2023-04-17.at.11.25.03.AM.mov

@sjcobb
Copy link
Member Author

sjcobb commented Apr 26, 2023

Separate from my previous note, the lines seem extra flicker-y with this change. My guess is that the highlight & downplay events you're triggering are still fighting with the highlight/trigger events from the axis pointer.

Using the nightly build, your fix in ECharts is definitely helping with the flicker!

One downside is when we set axisPointer.triggerEmphasis to false, the points that move with your cursor, stop showing. To adjust for this, I decided to bring back symbols when the chart renders, but only when there aren't a ton of datapoints (roughly correlates to when the dashboard time range is 15 minutes or less).

This was something I wanted to do anyways and it makes sense to go out with the highlighting improvements, since having better highlighting makes up for cases when symbols don't render (note: HIDE_DATAPOINTS_LIMIT condition is also important since rendering too many symbols can hurt performance).

@sjcobb
Copy link
Member Author

sjcobb commented Apr 26, 2023

you get this tooltip flicker of some items that occasionally have peaks near where you're moving the mouse left/right

I've adjusted variables that control how many series show and added a new SHOW_MORE_NEARBY_SERIES_LIMIT condition that will help with this. Now when there are 5 or less series, it will almost always show all 5, but when there are more series than will fit in the tooltip (w/o pinning and scrolling), the yBuffer area is smaller.

It doesn't completely address your concern, but between that and fixing the line color flicker using triggerEmphasis, it is a big improvement. I am planning to add panel options to override default tooltip behavior in the future. I need to sync with Mohit again, but these options could make sense:

  • Single Series
  • Nearby Series (Default)
  • All Series

@sjcobb sjcobb requested a review from cndonovan April 26, 2023 16:04
ui/components/package.json Outdated Show resolved Hide resolved
ui/components/src/TimeSeriesTooltip/focused-series.ts Outdated Show resolved Hide resolved
@@ -55,6 +57,14 @@ export function getLineSeries(
): EChartsTimeSeries {
const lineWidth = visual.line_width ?? DEFAULT_LINE_WIDTH;
const pointRadius = visual.point_radius ?? DEFAULT_POINT_RADIUS;

// Shows datapoint symbols when selected time range is roughly 15 minutes or less
let showPoints = data.length <= HIDE_DATAPOINTS_LIMIT;
Copy link
Contributor

Choose a reason for hiding this comment

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

What are datapoint symbols?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the little circle shown at points. It's called a "symbol" because you can use a variety of shapes. @sjcobb, correct me if I got that wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup that's right! Example of supported symbols, I went with 'circle' at the beginning instead of EChart's default emptyCircle since I ran benchmarks and circle is slightly faster

The big change here is now datapoints are visible when there aren't a ton of them (closer to how Grafana works now too)

image

ui/components/src/LineChart/LineChart.tsx Show resolved Hide resolved
ui/components/src/TimeSeriesTooltip/focused-series.ts Outdated Show resolved Hide resolved
@juliepagano
Copy link
Contributor

Still seeing a bit of flickering when you hover directly over lines. Any idea of that's something that can be worked around easily with some of the series emphasis settings? If not, I don't think it's too bad and can be revisited later.

Screen.Recording.2023-05-01.at.3.09.09.PM.mov

Copy link
Contributor

@juliepagano juliepagano left a comment

Choose a reason for hiding this comment

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

A couple small nits from me and Christine, but generally LGTM! Can you double-check with other folks that they're also ok with using the nightly before merging this?

@sjcobb
Copy link
Member Author

sjcobb commented May 5, 2023

Still seeing a bit of flickering when you hover directly over lines. Any idea of that's something that can be worked around easily with some of the series emphasis settings?

I'm seeing a tiny of flicker locally with that same graph, but it isn't nearly as bad as in your recording 😕
One small adjustment I just made is now emphasis.width is 0.5 smaller since the line did seem a little big on hover and you can still tell which are highlighted even when it's reduced slightly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants