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

Horizontal Scroll Bar Span Graph Timeline #2421

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

CoryBarney
Copy link

@CoryBarney CoryBarney commented Aug 29, 2024

Which problem is this PR solving?

Description of the changes

  • Adding horizontal scrollbar to gain a clickable interface to scrub though the trace timeline
  • Adding tests for said above addition

How was this change tested?

  • Manually and with added Jest
Jaeger.UI.-.Brave.2024-08-29.15-46-14.mp4

Checklist

- Adding Horizonal Scrollbar on timeline to give more user functionality on scrubbing spans
- Adding Tests for new functionality to check that it scrubs and that it is hidden when there are no anchor

Signed-off-by: Cory Barney <corybarney.cb@gmail.com>
@CoryBarney CoryBarney marked this pull request as ready for review August 29, 2024 19:38
@CoryBarney CoryBarney requested a review from a team as a code owner August 29, 2024 19:38
@CoryBarney CoryBarney requested review from joe-elliott and removed request for a team August 29, 2024 19:38
@CoryBarney CoryBarney marked this pull request as draft August 29, 2024 20:32
@CoryBarney
Copy link
Author

Got better direction on requirements making some changes

- Refactoring scrollbar to be top 40% of the visable anchor area of a selected time range

Signed-off-by: Cory Barney <corybarney.cb@gmail.com>
@yurishkuro
Copy link
Member

maybe a small bug - when there is a selection, the Hand icon for dragging only shows in the top part of the selection (correctly), but dragging also works if started outside of that selection in the top part (where hand icon is not shown). I think it's better to limit dragging to work as moving only within the selection.

@CoryBarney
Copy link
Author

Will do, thanks for pointing that out!

@CoryBarney
Copy link
Author

CoryBarney commented Aug 30, 2024

I think this is more in line with what your goal was, as well I noticed that the vertical red bar is present on the any place you can start dragging to create anchor points to I put that in. As well I synced up the cursor icon between the ViewingLayer and the TimelineViewingLayer so that they are the crosshair but I can swap that out to be whatever icon if you think the cross is no good.

Let me know and ill probably cut this into it's own PR as it kind of when on a tangent and don't want to overload it

Bug Detected: the redbar pops up when dragging but the cursor is present in the bottom 60% of the View

5ed169d_._api_v1_fail.simple_http_sentence.Jaeger.UI.-.Brave.2024-08-30.10-41-18.mp4

@CoryBarney
Copy link
Author

0a32ebc_._dispatch.frontend.Jaeger.UI.-.Brave.2024-08-30.12-14-50.mp4

Thats is the current kick so let me know if that is perfect and ill code up the tests for it

@yurishkuro
Copy link
Member

still seeing this bug #2421 (comment)

@CoryBarney
Copy link
Author

0a32ebc_._dispatch.frontend.Jaeger.UI.-.Google.Chrome.2024-08-30.21-05-31.mp4

I tried to replicate the functionality as close as possible to the google inspector and it has it global kind of like an invisible scroll bar. I can limit its functionality to just the physical range but I feel like being able to move and use your scroll wheel is a better UX.

@yurishkuro
Copy link
Member

ok, it's fine to do like Chrome does, but I see that in Chrome the drag indeed still works both over and outside of the current selection, but it also shows the hand icon in both cases (in the top part).

@CoryBarney
Copy link
Author

Currently it is setup to:

  • Show the hand icon open in the to 40%
  • The crosshair for making anchors in the bottom 60%
  • the red line which might be better to replace the crosshair with to be more in line
  • When clicked in the top 40% of the timeline scrubbing begins:
    • the icon is changed to a grab icon regardless of where it is in the web page until you release you click
    • Scrubbing moves left and right with the mouse curser regardless of where you are in the window until you release the click

Should I get a better recorder as it looks fine to me in first person without the weird none precision clicks that get shown in the recording? I'm grasping at straws what the bug is as the open hand icon is just present within the 40% and dragging closed hand is present to show you are still clicking regardless of where in the screen. I can try to cut it off for the upper section but I think that is a regression.

Am I misunderstanding something or having a difference in opinion (I do see the moment the crosshair comes up and will try to get the pixel sorted out)

08-31-2024_jaeger_pr_review.webm

@yurishkuro
Copy link
Member

so what I meant was:

  • when you have a selection and hover it, you have crosshair in the lower part, and hand in the upper part
  • but if you move the cursor horizontally outside of the selection, it turns into an arrow, even though the dragging functionality is the same as within the selection (top part: pan, bottom part: new selection)

It would be great to have the cursor to be consistently crosshair / hand when there is a selection (and just crosshair when there is no current selection).

- Refactoring to be an invisible scrubber part of the timeline viewer

Signed-off-by: Cory Barney <corybarney.cb@gmail.com>
@CoryBarney
Copy link
Author

i can swap on property on the css and swap what ever area to what needs be. I put everything back to default cursor and have the to range in the middle of the anchors a grab icon cause we need to let the users know that there is that functionality but I can remove it.

06adc24_._dispatch.frontend.Jaeger.UI.-.Brave.2024-09-01.14-15-59.mp4

@yurishkuro
Copy link
Member

Maybe I am not explaining correctly. I am concerned that these two positions have different icons:

image image

Since in the 2nd one the selection can still be panned by dragging, then for consistency it should be showing hand icon as well (when actually dragging it should change to fist in both cases).

In contrast, below I didn't expect a hand, because you are still doing selection (pointer cursor), not panning (hand/fist cursor)
image

Copy link

codecov bot commented Sep 14, 2024

Codecov Report

Attention: Patch coverage is 61.06195% with 44 lines in your changes missing coverage. Please review.

Project coverage is 95.85%. Comparing base (3b093f8) to head (bfa3cb5).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...acePage/TracePageHeader/SpanGraph/ViewingLayer.tsx 60.00% 44 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2421      +/-   ##
==========================================
- Coverage   96.61%   95.85%   -0.77%     
==========================================
  Files         254      254              
  Lines        7679     7765      +86     
  Branches     1997     2002       +5     
==========================================
+ Hits         7419     7443      +24     
- Misses        260      322      +62     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

[Feature]: Add Horizontal Scrollbar for Viewing Long Traces in Jaeger UI
2 participants