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

Add start time to marker tooltip #4468

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bruderstein
Copy link

@bruderstein bruderstein commented Feb 12, 2023

This adds the start time to the marker tooltip in the marker chart. The information was previously only shown in the marker table view, but adding it in the chart view makes it easier to assess if one thing happened before or after another.

I've done this as <duration> @ <start time>, but happy for any suggestions as to how to make this readable & obvious.
Screenshot 2023-02-12 at 20 41 33

If it's only a marker (i.e. no duration), then we only show the @ <start time>
Screenshot 2023-02-12 at 20 41 45

(First PR to this repo, always happy for feedback - i missed this feature whilst reviewing some profiles)

┆Issue is synchronized with this Jira Task

@codecov
Copy link

codecov bot commented Feb 12, 2023

Codecov Report

Base: 88.64% // Head: 88.64% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (b59ac29) compared to base (c05d0a3).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4468   +/-   ##
=======================================
  Coverage   88.64%   88.64%           
=======================================
  Files         284      284           
  Lines       25528    25529    +1     
  Branches     6860     6860           
=======================================
+ Hits        22629    22630    +1     
  Misses       2692     2692           
  Partials      207      207           
Impacted Files Coverage Δ
src/components/tooltip/Marker.js 98.46% <100.00%> (+0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@julienw
Copy link
Contributor

julienw commented Feb 17, 2023

Just a quick note that we've noticed your patch and even talked a bit about it, but I didn't have the chance to report back.
It's still on our radar though, thanks for your patience!

@bruderstein
Copy link
Author

No worries at all, no rush. Thanks so much for taking a look and commenting.

@julienw
Copy link
Contributor

julienw commented Mar 20, 2023

Hey @bruderstein ! Sorry for the long delay.
We discussed this and didn't like the solution a lot, but we see the value in having this information.
What do you think of adding this information as a "simple" property below the Thread/Page information?

@bruderstein
Copy link
Author

Yes, completely agree.

I can try to implement that, but feel free to do it if it's a 2 minute thing for someone and you don't want to wait.

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