-
Notifications
You must be signed in to change notification settings - Fork 114
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
Add com.snowplowanalytics.snowplow/mobile_context/jsonschema/1-0-3 (close #1252) #1253
Conversation
"isPortrait": { | ||
"type": ["boolean", "null"], | ||
"description": "A Boolean indicating whether the device orientation is portrait (either upright or upside down)" | ||
}, |
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.
Can you only get a boolean here, no way to understand the actual device orientation? (i.e. upsidedown)
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.
It is possible to understand the orientation in more detail but the problem is that iOS and Android report it in a different way and I wasn't able to find a mapping between the states.
Here is a table with the values, I was only able to confidently map the portrait positions but not sure whether the natural landscape on Android is left or right on iOS and there are also extra positions on each platform not on the other (such as face up or down).
iOS | Android | Description |
---|---|---|
portrait | portrait_primary | Home button at the bottom |
portraitUpsideDown | portrait_secondary | Home button at the top |
landscapeLeft | ? | Home button on the right side |
landscapeRight | ? | Home button on the left side |
faceUp | ? | parallel to the ground with the screen facing upwards |
faceDown | ? | parallel to the ground with the screen facing downwards |
? | landscape | can represent landscape-primary, landscape-secondary or both |
? | landscape_primary | natural orientation for landscape |
? | landscape_secondary | rotated 180° from its natural orientation |
? | natural | either portrait-primary or landscape-primary depending on the device's usual orientation |
? | portrait | portrait-primary, portrait-secondary or both |
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 am ok with the boolean since this is the current state ^^
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.
Oh Android.
I think a boolean is probably a good idea 😂
"resolution": { | ||
"type": ["string", "null"], | ||
"maxLength": 20, | ||
"description": "Screen resolution in pixels. Arrives in the form of WIDTHxHEIGHT (e.g., 1200x900)" | ||
}, |
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.
Given this is device information, I think I'd imagine it to stay static, but I'm curious to know if you'd expect it to change as device orientation changes or stay the same? Might be worth adding the behvaiour to the description.
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 point! It doesn't change with the orientation, I added a note in description.
cfadea7
to
50747ac
Compare
50747ac
to
f689c6d
Compare
@igneel64 @paulboocock I have added a couple of properties for the Android app set ID that we plan to add in this issue. Could you please take another look at the schema? Would be great to put together a release with |
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 am fine with the current state as well :)
Should we cherry pick the browser_context addition in this branch to mark it as the release ?
@igneel64 We usually make a new release PR where we cherry-pick the commits, let's wait for Paul regarding the comment he raised and we can go ahead with that 👍 |
Cherry picked into R140 #1256 |
Issue #1252
This is a new version of the
mobile_context
schema tracked by the mobile trackers which will add the following properties:The
resolution
,scale
, andlanguage
properties are added to reflect the newbrowser_context
on Web (#1249) which moves some of the canonical properties from the events to the context entity. This is another step on the path to deprecate the canonical properties and unify the Web and mobile data models.