jaeger-ui icon indicating copy to clipboard operation
jaeger-ui copied to clipboard

Horizontal Scroll Bar Span Graph Timeline

Open CoryBarney opened this issue 1 year ago • 14 comments

Which problem is this PR solving?

  • Resolves #2419

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

https://github.com/user-attachments/assets/e4ca58a4-4337-45f8-830e-da5221b613af

Checklist

  • [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
  • [x] I have signed all commits
  • [ ] I have added unit tests for the new functionality
  • [ ] I have run lint and test steps successfully
    • for jaeger: make lint test
    • for jaeger-ui: yarn lint and yarn test

CoryBarney avatar Aug 29 '24 19:08 CoryBarney

Got better direction on requirements making some changes

CoryBarney avatar Aug 29 '24 20:08 CoryBarney

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.

yurishkuro avatar Aug 29 '24 21:08 yurishkuro

Will do, thanks for pointing that out!

CoryBarney avatar Aug 29 '24 23:08 CoryBarney

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

https://github.com/user-attachments/assets/181e3c5e-ad57-4273-9312-fbd817ad1f99

CoryBarney avatar Aug 30 '24 13:08 CoryBarney

https://github.com/user-attachments/assets/ad5b016a-9062-458f-aa5b-0ea15a9cfe22

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

CoryBarney avatar Aug 30 '24 15:08 CoryBarney

still seeing this bug https://github.com/jaegertracing/jaeger-ui/pull/2421#issuecomment-2319111167

yurishkuro avatar Aug 30 '24 23:08 yurishkuro

https://github.com/user-attachments/assets/dde3ea22-3e29-402b-86a9-4a4660c417f0

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.

CoryBarney avatar Aug 31 '24 00:08 CoryBarney

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).

yurishkuro avatar Aug 31 '24 03:08 yurishkuro

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

CoryBarney avatar Aug 31 '24 16:08 CoryBarney

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).

yurishkuro avatar Aug 31 '24 17:08 yurishkuro

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.

https://github.com/user-attachments/assets/7a66fa3f-25d7-4a6d-89b1-c89fa8b52b78

CoryBarney avatar Sep 03 '24 02:09 CoryBarney

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

yurishkuro avatar Sep 03 '24 18:09 yurishkuro

Codecov Report

:x: Patch coverage is 61.06195% with 44 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 95.85%. Comparing base (3b093f8) to head (bfa3cb5). :warning: Report is 410 commits behind head on main.

Files with missing lines Patch % Lines
...acePage/TracePageHeader/SpanGraph/ViewingLayer.tsx 60.00% 44 Missing :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Sep 14 '24 19:09 codecov[bot]

I would like to work on this PR.

Srishti-j18 avatar Feb 02 '25 12:02 Srishti-j18

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. You may re-open it if you need more time.

github-actions[bot] avatar Jul 28 '25 02:07 github-actions[bot]

This pull request has been automatically closed due to inactivity. You may re-open it if you need more time. We really appreciate your contribution and we are sorry that this has not been completed.

github-actions[bot] avatar Aug 18 '25 02:08 github-actions[bot]