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: hold traces tooltip on hover #2387

Closed
wants to merge 5 commits into from
Closed

Conversation

palbizu
Copy link
Contributor

@palbizu palbizu commented Sep 8, 2023

Description

Hold tooltip on hover

Testing

Locally tested

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

@palbizu palbizu requested a review from a team as a code owner September 8, 2023 22:55
@palbizu palbizu requested a review from arjunlalb September 8, 2023 22:55
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #2387 (d96209d) into main (8605c42) will decrease coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##             main    #2387      +/-   ##
==========================================
- Coverage   82.99%   82.98%   -0.01%     
==========================================
  Files         921      921              
  Lines       20566    20570       +4     
  Branches     3245     3247       +2     
==========================================
+ Hits        17069    17071       +2     
- Misses       3375     3377       +2     
  Partials      122      122              
Files Coverage Δ
...nents/utils/chart-tooltip/chart-tooltip-popover.ts 100.00% <100.00%> (ø)
...d/components/cartesian/d3/chart/cartesian-chart.ts 66.66% <11.11%> (-0.34%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions

This comment has been minimized.

this.hideCrosshair();
this.tooltip?.hovered$
.pipe(
debounceTime(300),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the debounceTime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to provide a better user experience when hover over the graph with the cursor

@@ -30,8 +33,8 @@ export class ChartTooltipPopover<TData, TContext> implements ChartTooltipRef<TDa
this.popoverRef.updatePositionStrategy({
type: PopoverPositionType.FollowMouse,
boundingElement: element,
offsetX: 20,
offsetY: 30
offsetX: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the impact of this change at other places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will be changing the way that we display the tooltip keeping it visible when the user has the cursor on the tooltip but will not break the functionality.

@github-actions
Copy link

Unit Test Results

       4 files  ±0     311 suites  ±0   47m 22s ⏱️ - 3m 6s
1 127 tests ±0  1 127 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
1 137 runs  ±0  1 137 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit d96209d. ± Comparison against base commit 8605c42.

@palbizu palbizu closed this Sep 27, 2023
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.

2 participants