-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
report: fix wording when screenEmulation disabled #14587
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
report/renderer/util.js
Outdated
let deviceEmulation = Util.i18n.strings.runtimeMobileEmulation; | ||
if (settings.screenEmulation.disabled) { | ||
deviceEmulation = Util.i18n.strings.runtimeNoEmulation; | ||
} else if (settings.formFactor === 'desktop') { |
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 still think we should use settings.screenEmulation.mobile
here because the strings refer to the emulated device and not scoring methodology.
That should only matter when the formFactor
and screenEmulation.mobile
disagree for some reason so this is probably fine.
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.
Good idea.
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'); |
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 does bring it back to @connorjclark's point that there's no indication this was a mobile report now
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.
Adding a separate indicator for that would be fine, I just don't think it makes sense here.
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.
fixes #14560