✨ Introduces new amp-story-bookend element tag#14937
✨ Introduces new amp-story-bookend element tag#14937newmuis merged 7 commits intoampproject:masterfrom
Conversation
| mandatory_ancestor: "AMP-STORY" | ||
| mandatory_last_child: true | ||
| attrs: { | ||
| name: "bookend-config-src" |
There was a problem hiding this comment.
Can we rename this to just src?
|
|
||
| /** Class corresponding to the <amp-story-bookend> DOM element tag. */ | ||
| export class AmpStoryBookend extends AMP.BaseElement { | ||
|
|
There was a problem hiding this comment.
Can we do the loading of the bookend in this class?
There was a problem hiding this comment.
We can, it will just create some code duplication from amp-story-bookend.js to keep it backwards compatible.
There was a problem hiding this comment.
As discussed offline, I will be doing this on a separate PR.
| | </amp-story> | ||
| | </body> | ||
| | </html> | ||
| | </html> No newline at end of file |
There was a problem hiding this comment.
Did you expect to remove that end of file line break?
There was a problem hiding this comment.
I thought the test was expecting this but I don't think it does anymore. I'll keep it with a new line at the end.
| */ | ||
| loadJsonFromAttribute_(attributeName) { | ||
| if (!this.storyElement_.hasAttribute(attributeName)) { | ||
| const bookendElem = this.getBookendElement_(this.storyElement_); |
There was a problem hiding this comment.
Nit: we usually use bookendEl as a short for element
| } | ||
|
|
||
| getBookendElement_(element) { | ||
| return [].slice.call(element.children) |
There was a problem hiding this comment.
Nit: you can use the childElementsByTag from src/dom
return childElementsByTag(this.storyElement_, 'amp-story-bookend')[0];
You shouldn't need to pass the element as an argument here, since you're using that private property.
And jsdoc please : ))
There was a problem hiding this comment.
Even better, childElementByTag, since we're only getting one.
There was a problem hiding this comment.
Nice, thanks! I'll just make the call inline since I don't think it deserves a separate method anymore.
| } | ||
|
|
||
| getBookendElement_(element) { | ||
| return [].slice.call(element.children) |
There was a problem hiding this comment.
Even better, childElementByTag, since we're only getting one.
|
|
||
| /** @override */ | ||
| isLayoutSupported(layout) { | ||
| return layout == Layout.CONTAINER; |
| */ | ||
| loadJsonFromAttribute_(attributeName) { | ||
| if (!this.storyElement_.hasAttribute(attributeName)) { | ||
| const bookendEl = childElementsByTag(this.storyElement_, |
There was a problem hiding this comment.
Nit: childElementByTag(this.storyElement_, 'amp-story-bookend') like Alan suggested : ))
There was a problem hiding this comment.
Oops, didn't see that comment. Thanks!
| import {once} from '../../../src/utils/function'; | ||
| import {user} from '../../../src/log'; | ||
|
|
||
| // TODO(#14591): Replace with 'src' when bookend api v0.1 is deprecated. |
There was a problem hiding this comment.
This is in the 1.0 codebase, so you should just be able to replace this now
There was a problem hiding this comment.
(can you change this before merging?)
There was a problem hiding this comment.
Oh true. Ok, will do.
| import {once} from '../../../src/utils/function'; | ||
| import {user} from '../../../src/log'; | ||
|
|
||
| // TODO(#14591): Replace with 'src' when bookend api v0.1 is deprecated. |
There was a problem hiding this comment.
(can you change this before merging?)
|
Ready to ship! |
* Introduces new amp-story-bookend element tag with validator logic. * Changes attribute to src. * Reviews. * Change to nodisplay layout. * change to childElementTag. * Changes attribute to fetch JSON to src. * fix tests.
Continuation of #14613 and in preparation of #12167.
Introduction of the new amp-story-bookend tag. Which opens the door for potentially supporting bookends entirely built in the DOM, enabling publishers to specify inline JSON and obviating the need for the RPC.
This introductory PR accepts JSONs using v0.1 and v1.0 of the bookend, but we will eventually only accept JSONs using v1.0 of the bookend JSON API.