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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(content-width): not applicable on desktop #5893

Merged
merged 3 commits into from Sep 26, 2018

Conversation

patrickhulce
Copy link
Collaborator

https://twitter.com/gcmznt/status/1032188854130880512 got me thinking that since the audit is just useless in DevTools without emulation we should mark it as not applicable. The meta tag should still capture this concern on the desktop site, but honestly the whole PWA category should be taken with a grain of salt if you're not auditing with mobile (toplevel warning perhaps? or maybe the justification for category level warning 馃槃)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

but honestly the whole PWA category should be taken with a grain of salt if you're not auditing with mobile (toplevel warning perhaps? or maybe the justification for category level warning 馃槃)

Definitely. Top level warning might be the best idea until we have a real solution for this future world of ChromeOS/Windows desktop PWAs

const viewportWidth = artifacts.ViewportDimensions.innerWidth;
const windowWidth = artifacts.ViewportDimensions.outerWidth;
const widthsMatch = viewportWidth === windowWidth;

if (context.settings.disableDeviceEmulation) {
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't this be disabled if you're running on a real phone, though?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, yes it would. Maybe a HostUserAgent plus disableDeviceEmulation check? Probably worth rolling up into a generic context isMobile boolean

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OTOH, I'm somewhat convinced this is essentially a redundant check with the viewport one, so I'm not inclined to go crazy propping it up.

Copy link
Member

Choose a reason for hiding this comment

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

fwiw this does also not work for the lr-desktop preset.

but it looks like we're going to use viewport emulation for lr-desktop now, with viewport of 1366x768 and mobile:false, to fix a screenshot bug.

So we also have that situation to contend with. :)

@patrickhulce
Copy link
Collaborator Author

Seems like this has been referenced in a few issues as a fix, anyone else wanna take a look? :)

@paulirish
Copy link
Member

discussed a bit in the meeting.

seems like

  1. we rebase this on top of emulateFormFactor.
    1. if emulateFormFactor === desktop, then mark as not applicable.
    2. if emulateFormFactor === mobile, continue as planned
    3. if emulateFormFactor is 'none', then we look at the HostUserAgent artifact:
      1. if the UA is mobile (contains 'android' (lol?) ), continue as planned
      2. if its not mobile, mark as not applicable

I think this captures the cases we described above. that sound right?

@patrickhulce
Copy link
Collaborator Author

that sound right?

sounds great!

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

馃悂 馃悩 馃挜 !

@paulirish paulirish merged commit 72b59c5 into master Sep 26, 2018
@paulirish paulirish deleted the not_applicable_viewport branch January 11, 2019 02:44
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.

None yet

3 participants