-
Notifications
You must be signed in to change notification settings - Fork 356
#2060 Add reusable datatypes and examples supporting Paid Media #2061
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
base: master
Are you sure you want to change the base?
#2060 Add reusable datatypes and examples supporting Paid Media #2061
Conversation
9217174 to
ba1a1e6
Compare
ba1a1e6 to
0463359
Compare
anandphatak
left a comment
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.
Could you pl. also add the JIRA ticket number in the PR title? Also I think you need to update the PR number in the description.
| }, | ||
| "xdm:numberValue": { | ||
| "title": "Number Value", | ||
| "$ref": "https://ns.adobe.com/xdm/data/measure", |
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.
Any reason to use the data type Measure here? Do you also need the "id" field specified in the Measure? If not, then you could use "type": "number" rather than $ref to Measure data type; that way you won't need to have a nested "value" field under "numberValue".
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.
we thought we might need follow ADC schema on Analytics side. You are right, it turns out we don't need "id" field for paidmedia schema. I changed it back to number type.
| "title": "Creative Assets", | ||
| "description": "Media assets used in the creative", | ||
| "items": { | ||
| "$ref": "#/definitions/asset" |
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.
Do you reuse the "asset" block defined in the schema in the future? Wanted to find out why it was taken out from the items and defined as a separate block.
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 don't think we might reuse "asset" in the schema, should I move it in items?
| "description": "Data type representing creative assets and content for paid media ads", | ||
| "definitions": { | ||
| "creative": { | ||
| "properties": { |
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'd recommend to keep these fields inside an object that acts like a namespace, for example "xdm:paidMediaCreative". You can choose an appropriate name that does not clash; some of the fields defined in this datatype clash with other datatypes in global XDM (for example "xdm:createiveID", "xdm:body", "xdm:destinationURL".) By nesting inside an object will avoid collisions
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.
changed creative to paidMediaCreative to avoid collisions.
This reverts commit 0463359.
#2060 will involve multiple PRs to keep the review effort manageable.
This PR adds the initial base datatypes that will be reused across multiple schemas in future PRs to support Paid Media concepts.