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

馃И Add story ad rendering to inabox. #30782

Merged
merged 6 commits into from
Oct 23, 2020
Merged

馃И Add story ad rendering to inabox. #30782

merged 6 commits into from
Oct 23, 2020

Conversation

calebcordry
Copy link
Member

Introduces new logic to inabox that detects for story ad metadata, and if it exists, renders as a story ad.

Part of #27851

@amp-owners-bot
Copy link

Hey @danielrozenberg! These files were changed:

build-system/global-configs/experiments-config.json

getStoryAdMetadataFromDoc,
maybeCreateAttribution,
validateCtaMetadata,
} from '../../extensions/amp-story-auto-ads/0.1/story-ad-ui';
Copy link
Member Author

Choose a reason for hiding this comment

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

I know we discussed importing from extension being a little weird, but I wasn't sure where to put them. Happy to move to a new file path.

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm fine with this as long as the compiler does not complain

Copy link
Contributor

@rsimha rsimha Oct 26, 2020

Choose a reason for hiding this comment

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

@lannka @calebcordry Looks like this did break compilation. https://travis-ci.com/github/ampproject/amphtml/jobs/405408314#L418

Can we fix before this goes to beta tomorrow?

/cc @ampproject/build-cop

@calebcordry calebcordry changed the title 馃И Add add rendering to inabox. 馃И Add story ad rendering to inabox. Oct 21, 2020
Copy link
Member

@danielrozenberg danielrozenberg left a comment

Choose a reason for hiding this comment

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

Approval for build-system/

getStoryAdMetadataFromDoc,
maybeCreateAttribution,
validateCtaMetadata,
} from '../../extensions/amp-story-auto-ads/0.1/story-ad-ui';
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm fine with this as long as the compiler does not complain

@CLAassistant
Copy link

CLAassistant commented Oct 23, 2020

CLA assistant check
All committers have signed the CLA.

@calebcordry calebcordry merged commit 936b45b into ampproject:master Oct 23, 2020
@calebcordry calebcordry deleted the story-inabox branch October 23, 2020 17:03
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
* render inabox

* css

* experiment

* comments

* exp fix

* types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants