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

Extract the meta info formatting to a separate file #2617

Merged
merged 4 commits into from
Jul 3, 2020

Conversation

julienw
Copy link
Contributor

@julienw julienw commented Jul 1, 2020

I'll be using these new functions when displaying the information about saved profiles.

Deploy previews:

Look at both the profile info summary, and also inside the summary itself.

@codecov
Copy link

codecov bot commented Jul 1, 2020

Codecov Report

❗ No coverage uploaded for pull request base (main@9b30de5). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2617   +/-   ##
=======================================
  Coverage        ?   86.41%           
=======================================
  Files           ?      218           
  Lines           ?    17377           
  Branches        ?     4512           
=======================================
  Hits            ?    15016           
  Misses          ?     2162           
  Partials        ?      199           

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 9b30de5...48176e8. Read the comment docs.

function _formatMetainfoString(meta: ProfileMeta) {
const productAndVersion = formatProductAndVersion(meta);
const os = formatPlatform(meta);
return productAndVersion + (os ? ` – ${os}` : '');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept it here because in the future it will disappear: indeed we'll display the 2 parts in 2 different elements, because we'll also want to add small icons to represent the platforms and products. Very probably this will all go to a different component.

@julienw julienw force-pushed the extract-metainfo-formatting branch from 06b6cfe to b0251a7 Compare July 1, 2020 16:43
@julienw julienw requested a review from canova July 1, 2020 16:44
Copy link
Member

@canova canova left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and lots of deploy previews! Looks good to me with some nits and suggestions but implementation wise I like this version! A lot clearer with less distractions!
Also it would be good to test it with a Chrome profile. Not sure if we are putting version and platform information to chrome profiles.

src/components/app/MenuButtons/MetaInfo.js Outdated Show resolved Hide resolved
src/components/app/MenuButtons/MetaInfo.js Outdated Show resolved Hide resolved
src/profile-logic/profile-metainfo.js Show resolved Hide resolved
src/components/shared/WindowTitle.js Show resolved Hide resolved
src/profile-logic/profile-metainfo.js Outdated Show resolved Hide resolved
src/profile-logic/profile-metainfo.js Show resolved Hide resolved
src/profile-logic/profile-metainfo.js Show resolved Hide resolved
src/test/unit/profile-metainfo.test.js Outdated Show resolved Hide resolved
src/test/unit/profile-metainfo.test.js Show resolved Hide resolved
@julienw
Copy link
Contributor Author

julienw commented Jul 3, 2020

Thanks for the review!

@julienw julienw merged commit 5c9cfc7 into firefox-devtools:main Jul 3, 2020
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.

2 participants