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(webapp): add tooltip to main timeline in single view #1742

Merged
merged 6 commits into from
Nov 22, 2022

Conversation

eh-am
Copy link
Collaborator

@eh-am eh-am commented Nov 18, 2022

Overall

Closes #1739

Adds a tooltip when hovering over the timeline in SingleView
Screen Shot 2022-11-18 at 15 36 49

Implementation

Create a TimelineTooltip component which just takes the following props:

export interface TimelineTooltipProps {
  timeLabel: string;
  items: {
    color?: Color;
    value: string;
    label: string;
  }[];
}

And renders as is.

Point here is to move the calculations to the caller, since each page may have different values/formats (eg tag explorer).

Also refactors <ExploreTooltip> to use this new TimelineTooltip.

Ah, fixes #1740 too

Some other comments

As it's clear, the Tooltip plugin (from float) has an annoying api, forcing us to access items such as

          value: a?.closest?.[1],

It's a good idea to refactor that plugin.

@eh-am eh-am changed the title Feat/add tooltip to main timeline feat(webapp): add tooltip to main timeline in single view Nov 18, 2022
@eh-am eh-am added the frontend Mostly JS code label Nov 18, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 18, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
webapp/public/assets/app.js 492.84 KB (+0.08% 🔺) 9.9 s (+0.08% 🔺) 3.6 s (+6.77% 🔺) 13.5 s
webapp/public/assets/app.css 19.99 KB (-0.02% 🔽) 400 ms (-0.02% 🔽) 0 ms (+100% 🔺) 400 ms
webapp/public/assets/styles.css 9.56 KB (0%) 192 ms (0%) 0 ms (+100% 🔺) 192 ms
packages/pyroscope-flamegraph/dist/index.js 131.27 KB (0%) 2.7 s (0%) 1.8 s (-19.47% 🔽) 4.5 s
packages/pyroscope-flamegraph/dist/index.node.js 131.96 KB (0%) 2.7 s (0%) 641 ms (-23.56% 🔽) 3.3 s
packages/pyroscope-flamegraph/dist/index.css 8.29 KB (0%) 166 ms (0%) 0 ms (+100% 🔺) 166 ms

@codecov
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

Base: 66.38% // Head: 66.25% // Decreases project coverage by -0.13% ⚠️

Coverage data is based on head (77fcd36) compared to base (501b7f2).
Patch coverage: 93.03% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1742      +/-   ##
==========================================
- Coverage   66.38%   66.25%   -0.12%     
==========================================
  Files         168      169       +1     
  Lines        5522     5561      +39     
  Branches     1243     1260      +17     
==========================================
+ Hits         3665     3684      +19     
- Misses       1848     1867      +19     
- Partials        9       10       +1     
Impacted Files Coverage Δ
...e-flamegraph/src/FlameGraph/FlameGraphRenderer.tsx 50.91% <ø> (+0.91%) ⬆️
...ackages/pyroscope-flamegraph/src/ProfilerTable.tsx 56.37% <ø> (-4.53%) ⬇️
packages/pyroscope-flamegraph/src/Toolbar.tsx 86.37% <80.00%> (+3.16%) ⬆️
webapp/javascript/pages/exemplars/filterNonCPU.ts 93.34% <93.34%> (ø)
webapp/javascript/components/AppSelector/index.tsx 88.24% <100.00%> (ø)
webapp/javascript/pages/formatTableData.ts 77.42% <100.00%> (-18.23%) ⬇️
.../pyroscope-flamegraph/src/Tooltip/TableTooltip.tsx 47.28% <0.00%> (-10.86%) ⬇️
... and 5 more

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.

@eh-am eh-am marked this pull request as ready for review November 18, 2022 15:46
@petethepig
Copy link
Member

/create-server

@pavelpashkovsky
Copy link
Contributor

@eh-am can you please test this logic on short timeline period before merging
image

@eh-am
Copy link
Collaborator Author

eh-am commented Nov 21, 2022

/create-server

@eh-am eh-am merged commit 508946c into main Nov 22, 2022
@eh-am eh-am deleted the feat/add-tooltip-to-main-timeline branch November 22, 2022 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Mostly JS code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NaN in tag explorer tooltip Add tooltip to timeline on hover
3 participants