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

ui: Docked Graph MetaInfo Panel #3683

Merged
merged 14 commits into from
Sep 11, 2023
Merged

ui: Docked Graph MetaInfo Panel #3683

merged 14 commits into from
Sep 11, 2023

Conversation

manojVivek
Copy link
Contributor

@manojVivek manojVivek commented Aug 25, 2023

Screenshots:
Screenshot 2023-08-25 at 1 58 10 PM
Screenshot 2023-08-25 at 1 57 41 PM

@manojVivek manojVivek requested a review from a team as a code owner August 25, 2023 08:26
@alwaysmeticulous
Copy link

alwaysmeticulous bot commented Aug 25, 2023

🤖 Meticulous spotted visual differences in 16 of 322 screens tested: view and approve differences detected.

Last updated for commit d96b3fa. This comment will update as new commits are pushed.

Copy link
Contributor

@monicawoj monicawoj left a comment

Choose a reason for hiding this comment

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

The code looks good to me, but when looking at the Vercel preview, selecting the option to "Dock Graph Metainfo" doesn't seem to dock the tooltip to the bottom of the screen? Is that the expected behavior?

@manojVivek
Copy link
Contributor Author

The code looks good to me, but when looking at the Vercel preview, selecting the option to "Dock Graph Metainfo" doesn't seem to dock the tooltip to the bottom of the screen? Is that the expected behavior?

@monicawoj You need to enable Arrow Flamegraph as well.
Since we're going to phase out the non-arrow flamegraph I made this change only for the arrow version.

return `${valueFormatter(hoveringNodeCumulative, unit, 2)}
(${(100 * divide(hoveringNodeCumulative, totalUnfiltered)).toFixed(2)}%${filtered})`;
};
const {name, locationAddress, cumulativeText, diffText, diff, row: rowNo} = graphTooltipData;
Copy link
Contributor

Choose a reason for hiding this comment

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

Super minor comment, but could we rename "rowNo" to "rowNumber". "No" can be interpreted in multiple ways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, addressed it.

Copy link
Contributor

@monicawoj monicawoj left a comment

Choose a reason for hiding this comment

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

looks fantastic! just one tiny comment

@metalmatze
Copy link
Member

Just FYI, Arrow is the only and default API used for querying flame graphs now. Not sure this changes anything in this PR, but it might make things a littler easier?

monicawoj added a commit that referenced this pull request Sep 11, 2023
* Extracted the tooltip data logic into hooks for reuse on the new design

* Docked graph tooltip

* Dock and undock buttons right in the metainfo panel

* Lint error fixed

* Variable name fixed to be more meaningful

* Arrow string conversion

---------

Co-authored-by: Monica Wojciechowska <[email protected]>
monicawoj added a commit that referenced this pull request Sep 11, 2023
* add show hide legend button and url state

* align columns, show full name on hover

* hide legend button on compare mode and default color preferences

* change default color profile to ocean

* change display names of color palettes

* ui: Docked Graph MetaInfo Panel (#3683)

* Extracted the tooltip data logic into hooks for reuse on the new design

* Docked graph tooltip

* Dock and undock buttons right in the metainfo panel

* Lint error fixed

* Variable name fixed to be more meaningful

* Arrow string conversion

---------

Co-authored-by: Monica Wojciechowska <[email protected]>

* Publish

 - @parca/[email protected]
 - @parca/[email protected]
 - @parca/[email protected]
 - @parca/[email protected]
 - @parca/[email protected]
 - @parca/[email protected]
 - @parca/[email protected]

* remove duplicate object entry

* fix ts error and button props

---------

Co-authored-by: Manoj Vivek <[email protected]>
Co-authored-by: manojVivek <[email protected]>
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.

3 participants