core(base-artifacts): add FormFactorIsMobile base artifact #7280
Conversation
mattzeunert
requested review from
brendankenny,
patrickhulce and
paulirish
as code owners
mattzeunert
changed the title
mattzeunert
changed the title
There was a problem hiding this comment.
nice! LGTM, I'm not a huge fan of having both dangling but we can do another pass as part of #6747 before we make artifacts a real solidified API for folks
There was a problem hiding this comment.
looking good, though I only took an initial gander.
I know we were talking about an
Environmentartifact to wrap these two properties, but since there are already other environment-related properties directly onbaseArtifactsI just added them there.
I'm not a huge fan of having both dangling but we can do another pass as part of #6747
yeah, same, and I'm fine with waiting as well. We mostly don't consider artifacts to be part of our public API for semver yet (we will for v5 and plugins becoming a thing?), so the other environment props can get wrapped up then too.
types/artifacts.d.ts
Outdated
| @@ -23,6 +23,10 @@ declare global { | |||
| fetchTime: string; | |||
| /** A set of warnings about unexpected things encountered while loading and testing the page. */ | |||
| LighthouseRunWarnings: string[]; | |||
| /** Whether the page was loaded on either a real or emulated mobile device */ | |||
| IsMobile: boolean; | |||
There was a problem hiding this comment.
we might need to bikeshed on these. When I see artifacts.isMobile, my first thought is "is what mobile?" :)
It'll make a little more sense when nested in an environment artifact, but do we have other ideas?
(I also like reusing host for the UA string artifacts, but (super nitpicky) dislike the host prefix vs suffix :)
There was a problem hiding this comment.
IsMobileFormFactor?
Co-Authored-By: mattzeunert <matt@mostlystatic.com>
types/artifacts.d.ts
Outdated
| /** Whether the page was loaded on either a real or emulated mobile device. */ | ||
| IsMobile: boolean; | ||
| /** Whether Lighthouse was run on a mobile device (i.e. not on a desktop machine). */ | ||
| IsMobileHost: boolean; |
There was a problem hiding this comment.
apparently, this is false if it was real mobile hardware but desktop was emulated.
which isn't what I expected given the name.
There was a problem hiding this comment.
how about slightly different booleans?
isMobileHardware
isMobileEmulated
They will combine differently, but I think they communicate all the same things?
There was a problem hiding this comment.
how about slightly different booleans?
isMobileHardware
isMobileEmulated
That kind of just punts the question back to the audits again, though. Really we just want to be able to easily answer "were my artifacts gathered in a mobile environment, whether real or emulated?".
The value should be true if run on desktop with mobile emulation, run on mobile with mobile emulation, or run on mobile with no emulation, otherwise false.
Maybe we should just drop IsMobileHost for now (I don't think anything needs it at this point?) and just pick a name we all like for isMobile/IsMobileFormFactor/isMobileEmulated?
There was a problem hiding this comment.
I like isMobileHardware to replace isMobileHost 👍
when discussing @paulirish 's comment we just used the phrase isMobileConditions which I kinda like over isMobile/isMobileFormFactor. I'd want to avoid Emulated since that's not really what we are getting at
There was a problem hiding this comment.
isMobileHardware
wasMobileConditions
???
the second is past tense because it's sometimes true and sometimes not.
crazy but semantically reasonable?
There was a problem hiding this comment.
Renamed to FormFactorIsMobile and removed IsMobileHost from the base artifacts for now.
There was a problem hiding this comment.
at first read, I'd probably have to look up whether FormFactorIsMobile meant either tested on actual mobile hardware or the definition we're going for.
What about something around TestedAsMobileDevice?
There was a problem hiding this comment.
I'm starting to circle back around to @mattzeunert 's suggestion that this represents whether the device presented itself as mobile to the site, i.e network user agent was mobile :)
at this point though, I think we've all identified it's hard to name and we should just document well in the comment above it what it really represents
There was a problem hiding this comment.
user agent is independent of form factor, though, isn't it? That's why I was suggesting we go with whatever awkward string reasonably encapsulates RanLighthouseWithADeviceThatEitherWasOrPretendedToBeAMobileOne, which would include the UA string and screen characteristics
There was a problem hiding this comment.
TestedAsMobileDevice works for me, updated the PR.
mattzeunert
changed the title
|
We're using |
|
everyone else ok with |
WFM |
|
lgtm on naming :) |
Summary
Extract the logic for this from the
content-widthaudit so we can use it for other audits.@patrickhulce I know we were talking about an
Environmentartifact to wrap these two properties, but since there are already other environment-related properties directly onbaseArtifactsI just added them there.Related Issues/PRs
Closes #7043