PCF-562 Consider adding the OS version to the name of the platform in the "Platform" column #816
Conversation
✅ Deploy Preview for mozilla-perfcompare ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #816 +/- ##
==========================================
+ Coverage 92.62% 92.64% +0.01%
==========================================
Files 93 93
Lines 2577 2582 +5
Branches 515 516 +1
==========================================
+ Hits 2387 2392 +5
Misses 168 168
Partials 22 22 ☔ View full report in Codecov by Sentry. |
src/utils/platform.ts
Outdated
|
|
||
| if (platform.startsWith('windows')) | ||
| return `${platformShortName}\u00a0${platform | ||
| .split('-')[0] | ||
| .replace('windows', '')}`; | ||
| if (platform.startsWith('win')) | ||
| return `${platformShortName}\u00a0${platform | ||
| .split('-')[0] | ||
| .replace('win', '')}`; | ||
| if (platform.startsWith('macosx')) | ||
| return `${platformShortName}\u00a0${platform | ||
| .split('-')[0] | ||
| .replace('macosx', '')}`; | ||
| if (platform.startsWith('linux')) | ||
| return `${platformShortName}\u00a0${platform | ||
| .split('-')[0] | ||
| .replace('linux', '')}`; |
There was a problem hiding this comment.
to add comments with example
e56ebb9 to
f53c368
Compare
| return 'Unspecified'; | ||
| }; | ||
|
|
||
| const osMapping = { |
There was a problem hiding this comment.
nit: please add a comment with a link to your source (is that https://github.com/mozilla/treeherder/blob/master/ui/helpers/constants.js#L29 ?)
src/utils/platform.ts
Outdated
| }; | ||
|
|
||
| const extractPlatformWithOs = (platform: string) => { | ||
| const name = osMapping[platform.split('-')[0] as PlatformOS]; |
There was a problem hiding this comment.
I'd rewrite it this way as a slightly more readable alternative:
const system = platform.split('-')[0];
if (system in osMapping) {
return osMapping[system as PlatformOS];
}
return getPlatformShortName(platform);(optional)
src/types/types.ts
Outdated
| isFiltered: boolean; | ||
| }; | ||
|
|
||
| export type PlatformOS = |
There was a problem hiding this comment.
I'd remove the type from here, as it's used only once there's no reason to have an exported type. It's used only as a tool in platform.ts. So instead I'd do this in platform.ts:
type PlatformOS = keyof typeof osMapping;So that we don't have to maintain the data in both places.
src/utils/platform.ts
Outdated
| }; | ||
|
|
||
| export const getPlatformAndVersion = (platform: string): string => { | ||
| const platformShortName = getPlatformShortName(platform); |
There was a problem hiding this comment.
you can move the call to getPlatformShortName inside the if block now, as it's used only there.
(be careful to add the brackets :D)
f53c368 to
0ddc433
Compare
Updates: [Julien Wajsberg] Small adjustments on the sort functionality (#837) [esanuandra] PCF-562 Consider adding the OS version to the name of the platform in the "Platform" column (#816) [Carla Severe] Removed repetitive style references (#836) [Julien Wajsberg] Rename OSX to macOS to better reflect the newest naming for this platform (#840) [Julien Wajsberg] Extract the table filtering code to a common file (#839) [Julien Wajsberg] Implement sorting on the main results page (#838) [Chineta Adinnu] Add tooltips to revision column headers (#832) [Chineta Adinnu] Add aria expanded to revision row expand button (#826)
Jira link
Bugzilla link
Localhost testing link
Deploy testing link