Add the core amp-story component #11511
Conversation
| html.i-amphtml-story-standalone body, | ||
| html[amp-story] body, | ||
| html[📖] body { | ||
| display: grid !important; |
There was a problem hiding this comment.
Does the body need be grid as well?
There was a problem hiding this comment.
This is display: grid for a different reason; this is our current "desktop UI," which is just a centered mobile-phone-sized screen. This will be removed soon, when we have a true desktop UI (that is being developed now).
| max-width: 414px; | ||
| } | ||
|
|
||
| amp-story[standalone]:-webkit-full-screen, |
There was a problem hiding this comment.
Combine all fullscreen declarations below?
There was a problem hiding this comment.
Fun fact: you can't :(
Some browsers find other browsers' vendor prefixes to be illegal selectors, and instead of dropping just that part of the compound selector, it drops the whole thing. In other words, because :-moz-full-screen is present in part of the selector, Chrome would drop amp-story[standalone]:-webkit-fullscreen, amp-story[standalone]:-moz-full-screen
There was a problem hiding this comment.
In that case, you should check what our CSS compiler does. I believe it combines all such selectors automatically.
| animation-delay: 1.0s; | ||
| } | ||
|
|
||
| @keyframes grow { |
| } | ||
|
|
||
| .i-amphtml-story-bookend-article-meta { | ||
| font-family: 'Roboto', sans-serif; |
There was a problem hiding this comment.
Is this ok for a general support?
There was a problem hiding this comment.
If you mean the high level UX decision of using Roboto: amp-story has a pretty opinionated default UI as compared to other components.
If you mean whether the font will actually get loaded, we can consider adding an <amp-font> tag for it, internally to the component.
There was a problem hiding this comment.
I'd rather actually somehow do font-display:optional thing for this. But it's ok.
There was a problem hiding this comment.
But hows does Roboto gets included on pages? Forced by validator? Or we just fallback?
| * @param {string} targetPageId | ||
| * @return {!Promise} | ||
| */ | ||
| // TODO: Update history state |
|
Added |
|
Adding @choumx, since he's on all of the other reviews. This seems good to go as long as:
|
|
|
||
| /** | ||
| * @param {string} id The ID of the page to be retrieved. | ||
| * @return {!AmpStoryPage} Retrieves the page with the specified ID. |
|
|
||
| /** | ||
| * @param {!Element} el | ||
| * @return {boolean} |
| * @return {!AmpStoryPage} Retrieves the page with the specified ID. | ||
| */ | ||
| getPageById_(id) { | ||
| return user().assert(this.pages_.find(page => page.element.id === id), |
There was a problem hiding this comment.
Use array.js#findIndex. Array#find doesn't have IE support and I don't think we polyfill it.
| .expandAsync(user().assertString(rawUrl)) | ||
| .then(url => Services.xhrFor(this.win).fetchJson(url)) | ||
| .then(response => { | ||
| user().assert(response.ok, 'Invalid HTTP response'); |
There was a problem hiding this comment.
Nit: Add more error info for user debugging.
|
|
||
| /** | ||
| * @return {!Promise<?./bookend.BookendConfig>} | ||
| */ |
| .then(response => response && { | ||
| shareProviders: response['share-providers'], | ||
| relatedArticles: response['related-articles'] ? | ||
| buildFromJson(response['related-articles']) : [], |
There was a problem hiding this comment.
Minor: Can we change buildFromJson to just return [] on undefined input?
| * @param {string} pageId The page to be added to the map. | ||
| * @return {!Object<string, number>} A mapping from page ID to the priority of | ||
| * that page. | ||
| */ |
| */ | ||
| getPagesByDistance_() { | ||
| const distanceMap = this.getPageDistanceMapHelper_( | ||
| /* distance */ 0, /* map */ {}, this.activePage_.element.id); |
There was a problem hiding this comment.
First argument is priority not distance.
There was a problem hiding this comment.
The variable name was the one that was wrong
| const nextScreenAreaMin = this.element./*OK*/offsetLeft + | ||
| ((1 - NEXT_SCREEN_AREA_RATIO) * this.element./*OK*/offsetWidth); | ||
| const nextScreenAreaMax = this.element./*OK*/offsetLeft + | ||
| this.element./*OK*/offsetWidth; |
There was a problem hiding this comment.
Nit: Hoist these into local vars for readability.
const offsetWidth = this.element./*OK*/offsetWidth;| return; | ||
| } | ||
|
|
||
| map = this.getPageDistanceMapHelper_(priority + 1, map, adjacentPageId); |
There was a problem hiding this comment.
Assignment to map (and I suppose the return value) doesn't seem necessary.
| amp-story { | ||
| height: 100% !important; |
There was a problem hiding this comment.
let's force box-sizing: border-box; to ensure height and width 100% are still effective if authors decide to add padding/border
There was a problem hiding this comment.
| html.i-amphtml-story-standalone body, | ||
| html[amp-story] body, | ||
| html[📖] body { | ||
| height: 100% !important; |
| text-transform: uppercase; | ||
| font-size: 10px; | ||
| padding: 0 0 8px; | ||
| margin: 48px 0 8px; |
There was a problem hiding this comment.
this probably needs a RTL version to reverse the left/right numbers
| } | ||
|
|
||
| .i-amphtml-story-share-list > li:first-child { | ||
| padding-left: 32px; |
There was a problem hiding this comment.
ditto, this and below need rtl rules to reset left to 0 and apply 32px to right
|
🎉 |
Depends on a number of other PRs: #11502 #11484 #11485 #11506 #11487 #11509 #11510