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

Make sidebar links #489

Merged
merged 5 commits into from
Nov 5, 2021
Merged

Make sidebar links #489

merged 5 commits into from
Nov 5, 2021

Conversation

clovis1122
Copy link
Contributor

@clovis1122 clovis1122 commented Oct 31, 2021

This PR uses the NavLink component on the sidebar to make these act as links. Because the history state is now managed by a 3rd party, we can safely remove the previous test as well (otherwise we would be basically testing the 3rd party library...).

closes #305

@codecov
Copy link

codecov bot commented Nov 1, 2021

Codecov Report

Merging #489 (9766147) into main (29c0153) will increase coverage by 0.80%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #489      +/-   ##
==========================================
+ Coverage   55.26%   56.06%   +0.80%     
==========================================
  Files         163      164       +1     
  Lines        6868     6990     +122     
  Branches      169      196      +27     
==========================================
+ Hits         3795     3918     +123     
+ Misses       2758     2755       -3     
- Partials      315      317       +2     
Impacted Files Coverage Δ
webapp/javascript/components/Sidebar.jsx 60.72% <60.00%> (-9.28%) ⬇️
...ponents/FlameGraph/FlameGraphComponent/testData.ts 100.00% <0.00%> (ø)
webapp/javascript/components/ProfilerHeader.jsx
webapp/javascript/components/Toolbar.tsx 90.68% <0.00%> (ø)
...pp/javascript/components/ProfilerHeader.module.css 66.67% <0.00%> (ø)
...omponents/FlameGraph/FlameGraphComponent/index.tsx 88.75% <0.00%> (+0.60%) ⬆️
...mponents/FlameGraph/FlameGraphComponent/Header.tsx 88.89% <0.00%> (+1.39%) ⬆️
pkg/exec/cli.go 49.67% <0.00%> (+2.12%) ⬆️
...lameGraph/FlameGraphComponent/Flamegraph_render.ts 96.97% <0.00%> (+2.28%) ⬆️
pkg/exec/cli_unix.go 14.76% <0.00%> (+3.44%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29c0153...9766147. Read the comment docs.

@clovis1122
Copy link
Contributor Author

@eh-am can I get a review for this one too?

@eh-am
Copy link
Collaborator

eh-am commented Nov 2, 2021

@clovis1122 Thank you again!

Hmm, weird, now the query params aren't being passed between different pages:
This is the main branch
Peek 2021-11-02 11-12

This is this PR
Peek 2021-11-02 11-13

@clovis1122
Copy link
Contributor Author

@eh-am whoops! Fixed!

Copy link
Collaborator

@eh-am eh-am left a comment

Choose a reason for hiding this comment

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

Thank you!

Added few suggestions, super minor, but overall looks great!

webapp/javascript/components/Sidebar.jsx Outdated Show resolved Hide resolved
webapp/javascript/components/Sidebar.jsx Outdated Show resolved Hide resolved
webapp/javascript/components/Sidebar.jsx Outdated Show resolved Hide resolved
@clovis1122
Copy link
Contributor Author

Thanks, done!

@eh-am eh-am merged commit 44c5ac0 into grafana:main Nov 5, 2021
@Rperry2174
Copy link
Contributor

This is awesome thanks @clovis1122 !!!

korniltsev pushed a commit that referenced this pull request Jul 18, 2023
* Update jpprof to 0.1.6

* Fix gradle problem with snapshot
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.

Make sidebar items actual links so that they can be opened in new tabs
3 participants