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.

Sign up for GitHub

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

Jump to bottom

report: fix wording when screenEmulation disabled #14587

Merged
merged 5 commits into from
Dec 8, 2022
Merged

report: fix wording when screenEmulation disabled #14587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
81e31c0
report: fix wording when screenEmulation disabled
paulirish Dec 5, 2022
6fd4825
errbody loves tests
paulirish Dec 5, 2022
4f8b8dc
build copy
paulirish Dec 5, 2022
3539870
flow report side and tests
paulirish Dec 5, 2022
23cf4d2
use screenEmu.mobile rather
paulirish Dec 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions core/util.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,10 +516,12 @@ class Util {
summary = cpuThrottling = networkThrottling = Util.i18n.strings.runtimeUnknown;
}

const deviceEmulation = {
mobile: Util.i18n.strings.runtimeMobileEmulation,
desktop: Util.i18n.strings.runtimeDesktopEmulation,
}[settings.formFactor] || Util.i18n.strings.runtimeNoEmulation;
let deviceEmulation = Util.i18n.strings.runtimeMobileEmulation;
if (settings.screenEmulation.disabled) {
deviceEmulation = Util.i18n.strings.runtimeNoEmulation;
} else if (!settings.screenEmulation.mobile) {
deviceEmulation = Util.i18n.strings.runtimeDesktopEmulation;
}

const screenEmulation = settings.screenEmulation.disabled ?
undefined :
Expand Down
25 changes: 23 additions & 2 deletions flow-report/test/sidebar/sidebar-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,17 @@ describe('SidebarRuntimeSettings', () => {
throughputKbps: 1.6 * 1024,
rttMs: 150,
},
screenEmulation: {disabled: true},
screenEmulation: {
disabled: false,
width: 200,
height: 200,
deviceScaleFactor: 3,
mobile: true,
},
} as any;
const root = render(<SidebarRuntimeSettings settings={settings}/>, {wrapper});

expect(root.getByText('Emulated Moto G4')).toBeTruthy();
expect(root.getByText('Emulated Moto G4 - 200x200, DPR 3')).toBeTruthy();
expect(root.queryByText('Emulated Moto G4 -')).toBeFalsy();
expect(root.getByText('Slow 4G throttling')).toBeTruthy();
expect(root.getByText('4x slowdown'));
Expand All @@ -83,6 +89,7 @@ describe('SidebarRuntimeSettings', () => {
screenEmulation: {
width: 100,
height: 100,
mobile: false,
deviceScaleFactor: 2,
},
} as any;
Expand All @@ -92,4 +99,18 @@ describe('SidebarRuntimeSettings', () => {
expect(root.getByText('Custom throttling')).toBeTruthy();
expect(root.getByText('1x slowdown'));
});

it('displays runtime settings when screenEmulation disabled', async () => {
const settings = {
formFactor: 'mobile',
throttlingMethod: 'provided',
throttling: {},
screenEmulation: {disabled: true},
} as any;
const root = render(<SidebarRuntimeSettings settings={settings}/>, {wrapper});

expect(root.getByText('No emulation')).toBeTruthy();
expect(root.queryByText('Emulated Moto G4 -')).toBeFalsy();
expect(root.getByText('Provided by environment')).toBeTruthy();
});
});
10 changes: 6 additions & 4 deletions report/renderer/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,10 +512,12 @@ class Util {
summary = cpuThrottling = networkThrottling = Util.i18n.strings.runtimeUnknown;
}

const deviceEmulation = {
mobile: Util.i18n.strings.runtimeMobileEmulation,
desktop: Util.i18n.strings.runtimeDesktopEmulation,
}[settings.formFactor] || Util.i18n.strings.runtimeNoEmulation;
let deviceEmulation = Util.i18n.strings.runtimeMobileEmulation;
if (settings.screenEmulation.disabled) {
deviceEmulation = Util.i18n.strings.runtimeNoEmulation;
} else if (!settings.screenEmulation.mobile) {
deviceEmulation = Util.i18n.strings.runtimeDesktopEmulation;
}

const screenEmulation = settings.screenEmulation.disabled ?
undefined :
Expand Down
13 changes: 7 additions & 6 deletions report/test/renderer/util-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,13 @@ describe('util helpers', () => {
});

it('builds device emulation string', () => {
const get = opts => Util.getEmulationDescriptions({
...opts,
screenEmulation: {disabled: true},
}).deviceEmulation;
assert.equal(get({formFactor: 'mobile'}), 'Emulated Moto G4');
assert.equal(get({formFactor: 'desktop'}), 'Emulated Desktop');
const get = settings => Util.getEmulationDescriptions(settings).deviceEmulation;
/* eslint-disable max-len */
assert.equal(get({formFactor: 'mobile', screenEmulation: {disabled: false, mobile: true}}), 'Emulated Moto G4');
assert.equal(get({formFactor: 'mobile', screenEmulation: {disabled: true, mobile: true}}), 'No emulation');
Copy link
Member

Choose a reason for hiding this comment

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

this does bring it back to @connorjclark's point that there's no indication this was a mobile report now

Copy link
Member

Choose a reason for hiding this comment

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

Adding a separate indicator for that would be fine, I just don't think it makes sense here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

#8178

assert.equal(get({formFactor: 'desktop', screenEmulation: {disabled: false, mobile: false}}), 'Emulated Desktop');
assert.equal(get({formFactor: 'desktop', screenEmulation: {disabled: true, mobile: false}}), 'No emulation');
/* eslint-enable max-len */
});

it('builds throttling strings when provided', () => {
Expand Down