-
Notifications
You must be signed in to change notification settings - Fork 450
Cleanup the profile info panel for size profiles #5657
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5657 +/- ##
==========================================
- Coverage 85.63% 85.58% -0.06%
==========================================
Files 312 312
Lines 30867 30918 +51
Branches 8488 8532 +44
==========================================
+ Hits 26434 26462 +28
- Misses 4009 4026 +17
- Partials 424 430 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| {meta.endTime && meta.startTime !== undefined ? ( | ||
| <div className="metaInfoRow"> | ||
| <span className="metaInfoLabel"> | ||
| <Localized id="MenuButtons--metaInfo--file-size"> | ||
| File size: | ||
| </Localized> | ||
| </span> | ||
| {formatBytes(meta.endTime - meta.startTime)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer a separate fileSize property over this. Yes it's somewhat consistent but it's also quite strange to read.
| {meta.fileName ? ( | ||
| <div className="metaInfoRow"> | ||
| <span className="metaInfoLabel"> | ||
| <Localized id="MenuButtons--metaInfo--file-name"> | ||
| File name: | ||
| </Localized> | ||
| </span> | ||
| {meta.fileName} | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you feel about showing File name for all types of profiles, if the property is present?
| {meta.product || | ||
| meta.profilingStartTime || | ||
| meta.updateChannel || | ||
| meta.appBuildID || | ||
| meta.debug !== undefined || | ||
| meta.extensions || | ||
| meta.arguments ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a long check, same with the "shouldShowPlatformSection" check below. Please break up this function into multiple this._renderXYZ() functions, one per section. Then we can have this check as an early return.
…name, hide empty sections).
0f0b78b to
c642a4d
Compare
mstange
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks!
Changes: [Nazım Can Altınova] Use getEmptySourceTable instead of constructing the table manually (#5641) [Florian Quèze] Source view for marker stacks (#5633) [Nazım Can Altınova] Add fallback to funcTable.lineNumber when computing line timings (#5644) [Florian Quèze] Show the marker keys instead of the labels in marker tooltips when alt is pressed. (#5625) [Francesco Lodolo] Add GitHub workflow to check Fluent reference files (#5647) [Nazım Can Altınova] Switch from Gitpod to GitHub Codespaces (#5653) [Nazım Can Altınova] Migrate from CircleCI to GitHub Actions (#5654) [Nazım Can Altınova] Run the CI in the production branch and on every pull request (#5661) [Nazım Can Altınova] Convert l10n-sync.js from Flow comment types to JSDoc comments for Typescript (#5659) [Florian Quèze] Display correct unit in Stack Chart tooltips for size profiles. (#5658) [Florian Quèze] Hide track management UI in single track profiles. (#5656) [Florian Quèze] Rename hiddenTrackCount to trackCount. (#5665) [Florian Quèze] Cleanup the profile info panel for size profiles (show file size and name, hide empty sections). (#5657) [Markus Stange] Use valibot for symbolication API response validation (#5666) [Nazım Can Altınova] Make sure to fetch the full git history for l10n-sync job in CI (#5667) [Nazım Can Altınova] Do not switch to call tree when clicking the activity graph while on sample based panels (#5672) And thanks to our localizers: be: Mikalai Udodau de: Michael Köhler el: Jim Spentzos en-GB: Ian Neal es-CL: ravmn fr: Théo Chevalier fy-NL: Fjoerfoks ia: Melo46 it: Francesco Lodolo [:flod] nl: Mark Heijl pt-BR: Marcelo Ghelman ru: berry, Valery Ledovskoy sv-SE: Andreas Pettersson tr: Grk zh-CN: Olvcpr423, wxie zh-TW: Pin-guang Chen
Example profile with the fileName field: https://share.firefox.dev/4qL68wH
Deploy preview: https://deploy-preview-5657--perf-html.netlify.app/public/1hf6rzbjx1dspn1v4eqcgnybtzzczf2f889ce10/stack-chart/