Skip to content

Conversation

@dogfrogfog
Copy link
Contributor

@dogfrogfog dogfrogfog commented Nov 10, 2022

Brief

Changes

  • add Fit Mode to Context Menu (flamegraph right click menu)

demo link: https://pr-1698.pyroscope.io/?name=hotrod.python.frontend%7B%7D&query=rideshare-app-golang.cpu%7B%7D

Screen.Recording.2022-11-10.at.14.59.14.mov

Concerns

  • need to add icons

@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
webapp/public/assets/app.js 490.47 KB (+0.02% 🔺) 9.9 s (+0.02% 🔺) 4 s (+9.36% 🔺) 13.8 s
webapp/public/assets/app.css 19.3 KB (+0.13% 🔺) 386 ms (+0.13% 🔺) 0 ms (+100% 🔺) 386 ms
webapp/public/assets/styles.css 9.52 KB (0%) 191 ms (0%) 0 ms (+100% 🔺) 191 ms
packages/pyroscope-flamegraph/dist/index.js 131.84 KB (+0.14% 🔺) 2.7 s (+0.14% 🔺) 1.7 s (-31.53% 🔽) 4.3 s
packages/pyroscope-flamegraph/dist/index.node.js 132.47 KB (+0.1% 🔺) 2.7 s (+0.1% 🔺) 615 ms (-20.72% 🔽) 3.3 s
packages/pyroscope-flamegraph/dist/index.css 8.3 KB (+0.3% 🔺) 167 ms (+0.3% 🔺) 0 ms (+100% 🔺) 167 ms

@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Base: 66.22% // Head: 66.26% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (2ff86e3) compared to base (8d92b0a).
Patch coverage: 75.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1698      +/-   ##
==========================================
+ Coverage   66.22%   66.26%   +0.04%     
==========================================
  Files         164      165       +1     
  Lines        5441     5464      +23     
  Branches     1220     1224       +4     
==========================================
+ Hits         3603     3620      +17     
- Misses       1831     1837       +6     
  Partials        7        7              
Impacted Files Coverage Δ
.../FlameGraph/FlameGraphComponent/styles.module.scss 61.54% <ø> (ø)
...e-flamegraph/src/FlameGraph/FlameGraphRenderer.tsx 49.41% <ø> (-0.29%) ⬇️
...graph/src/FlameGraph/FlameGraphComponent/index.tsx 81.64% <75.00%> (-0.25%) ⬇️
webapp/javascript/components/Sidebar.tsx 86.67% <0.00%> (-0.14%) ⬇️
...ebapp/javascript/components/SidebarCustomIcons.tsx 78.58% <0.00%> (ø)

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.

@dogfrogfog
Copy link
Contributor Author

/create-server

@dogfrogfog dogfrogfog marked this pull request as ready for review November 10, 2022 14:02
@dogfrogfog dogfrogfog requested a review from eh-am November 10, 2022 14:09
return (
<MenuItem key="fit-mode" onClick={handleClick}>
{/* todo: add icons */}
Fit mode: {isHeadFirst ? 'tail first' : 'heat first'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Fit mode: {isHeadFirst ? 'tail first' : 'heat first'}
Fit mode: {isHeadFirst ? 'tail first' : 'head first'}

Although since we have enough space, I think we can be more explicit Fit {head, tail} first or or some better wording cc @Rperry2174

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the wording is fine, but for consistency we should add the icon the same way the other options have an icon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

absolutely..which one should i use ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets use the same icons from the toolbar (depending on whether it says "head" or "tail" first. As far as the text for the other ones we kind of explain what happens with the option:
image

What do you guys think about:

  • "[ Icon ] Show text head first"
  • "[ Icon] Show text tail first"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks good i think

Screen.Recording.2022-11-14.at.11.04.11.mov

…nt/index.tsx

Co-authored-by: eduardo aleixo <eh-am@users.noreply.github.com>
@Ivanikitin Ivanikitin added the frontend Mostly JS code label Nov 10, 2022
@petethepig
Copy link
Collaborator

/create-server

@dogfrogfog
Copy link
Contributor Author

/create-server

@eh-am eh-am merged commit 082a971 into main Nov 14, 2022
@eh-am eh-am deleted the feature/add-tail-head-first-func-to-context-menu branch November 14, 2022 10:26
@eh-am
Copy link
Collaborator

eh-am commented Nov 14, 2022

Looks good! Thanks

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.

5 participants