Skip to content

[amp-story] Allows inline json config for bookend and share providers ✨#19171

Merged
gmajoulet merged 5 commits intoampproject:masterfrom
Enriqe:local-config-bookend
Nov 12, 2018
Merged

[amp-story] Allows inline json config for bookend and share providers ✨#19171
gmajoulet merged 5 commits intoampproject:masterfrom
Enriqe:local-config-bookend

Conversation

@Enriqe
Copy link
Copy Markdown
Contributor

@Enriqe Enriqe commented Nov 7, 2018

Closes #15826

@Enriqe Enriqe force-pushed the local-config-bookend branch from 915fac0 to 628bbd8 Compare November 7, 2018 16:23
@Enriqe Enriqe requested review from gmajoulet and newmuis November 7, 2018 16:26
@newmuis
Copy link
Copy Markdown
Contributor

newmuis commented Nov 7, 2018

What happens if I specify a src and an inline config?

@gmajoulet
Copy link
Copy Markdown
Contributor

In amp-story-share:

getInlineConfig_() {
    const bookend = this.storyEl.querySelector('amp-story-bookend');
    const inlineConfig = bookend.querySelector('script');

   ...
}

I think we should avoid these hard dependencies. Unfortunately here, I don't know how we can keep amp-story-share / amp-story-bookend communication going through the store service. There are cases where we don't want to load the bookend configuration before actually preloading the bookend (mobile version on a phone that supports native sharing doesn't need to know about the share providers).

Maybe we should refactor the request-service to support this feature, since it already queries the DOM for amp-story-bookend anyway.

src would have the priority over the inline config, since it might be more fresh? Having an inline config to make things easier, or as a fallback, is a really cool feature.

@Enriqe
Copy link
Copy Markdown
Contributor Author

Enriqe commented Nov 7, 2018

What happens if I specify a src and an inline config?

src would have the priority over the inline config, since it might be more fresh? Having an inline config to make things easier, or as a fallback, is a really cool feature.

I was thinking that if both are specified then we'd want to prioritize the inline config so that we avoid doing unnecessary XHR's. But what you mention sounds like a better UX/devX.

Maybe we should refactor the request-service to support this feature, since it already queries the DOM for amp-story-bookend anyway.

Yeah, now that we think of the inline config as a fallback we can refactor the request service to get the inline config as a fallback when there's no src specified.

I'll update the PR with these points taken into consideration.

@Enriqe
Copy link
Copy Markdown
Contributor Author

Enriqe commented Nov 8, 2018

/cc @honeybadgerdontcare @Gregable for validation

@gmajoulet gmajoulet merged commit ab609a8 into ampproject:master Nov 12, 2018
alin04 added a commit to alin04/amphtml that referenced this pull request Nov 13, 2018
@alin04 alin04 mentioned this pull request Nov 13, 2018
alin04 added a commit that referenced this pull request Nov 13, 2018
* cl/220306609 Revision bump for #17907

* cl/220307253 Revision bump for #19128

* cl/220310523 Revision bump for #19129

* cl/220399983 Revision bump for #19167

* cl/221145203 n/a

* cl/221159765 Revision bump for #19214

* cl/221164382 Invalidate `<amp-script>` tag as well

* cl/221176616 Revision bump for #17939

* cl/221181356 Revision bump for #19171
Enriqe added a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
… ✨ (ampproject#19171)

* allows inline json config for bookend and share providers

* types, childjsoncofig

* move logic to request service

* move logic inline loadBookendConfigImpl

* reviews
Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
* cl/220306609 Revision bump for ampproject#17907

* cl/220307253 Revision bump for ampproject#19128

* cl/220310523 Revision bump for ampproject#19129

* cl/220399983 Revision bump for ampproject#19167

* cl/221145203 n/a

* cl/221159765 Revision bump for ampproject#19214

* cl/221164382 Invalidate `<amp-script>` tag as well

* cl/221176616 Revision bump for ampproject#17939

* cl/221181356 Revision bump for ampproject#19171
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.

5 participants