✨ New amp-story-bookend components: portrait #15142

Enriqe · 2018-05-07T23:19:32Z

Adds new portrait card for the bookend components.
Introduces component container i-amphtml-story-bookend-component-set.
Changes article-set-title to heading per Rename "article-set-title" bookend component to "heading" #15088 and moves it to its own file.

gmajoulet · 2018-05-10T00:21:46Z

From your screenshot, we can see a few issues:

we could better position the image inside the its container, to crop as little as possible. Right now we only see the top left corner of the image
The portrait image is not full width

I tried to break the layout in all possible ways, and I think they're all issues we might want to fix:

Titles line height
Handling long domain names
Making sure the images fill their containers, and cropping as little as possible
I'm not sure about the portrait's title font, it is very hard to read :(

I think building a prototype on codepen or any other environment could help figuring out how you can better structure the CSS to handle the crazy cases publishers could come up with. Tweaking CSS rules to fix these bugs one by one without thinking about it as a whole could introduce even more issues, and will drive you crazy.

If you need any help with this we can book some time this week!

Enriqe · 2018-05-10T14:26:15Z

Thanks for the feedback @gmajoulet !

we could better position the image inside the its container, to crop as little as possible. Right now we only see the top left corner of the image.

Are referring to the small thumbnail image? If so, should we fix this in a separate PR since it's another issue different from the new component?

The portrait image is not full width

Thats true, but taking into account support for larger screen sizes and responsive layout, I think we should define a max-width so that no matter how large the screen is, at one point it should not grow in width. But I guess we could use columns for this and make it always take 100% width in its own column:

I'm not sure about the portrait's title font, it is very hard to read :(

Ok, should we increase the font size?

Meanwhile I will work on the fix for the long titles / domain names and looking into building a prototype to better accommodate the css!

newmuis · 2018-05-10T14:33:51Z

I don't think this is the "title" but rather the "eyebrow" as it was called in the mocks. The other components (with titles) have this too. I don't believe this component has a title.

Enriqe · 2018-05-10T14:51:15Z

That's correct @newmuis

gmajoulet · 2018-05-10T15:29:34Z

Very sorry to be picky, but I believe these will only take a few minutes and will greatly impact our product/design :))
I'm available to help you offline if you have to submit this quickly.

Are referring to the small thumbnail image? If so, should we fix this in a separate PR since it's another issue different from the new component?

Oh you're right, it's actually currently broken. The fix is as simple as adding height: 100%; width: 100%; on the <img> that has the object-fit: cover;, it might be worth adding it : )
Same image before and after

The portrait image is not full width

Thats true, but taking into account support for larger screen sizes and responsive layout, I think we should define a max-width so that no matter how large the screen is, at one point it should not grow in width. But I guess we could use columns for this and make it always take 100% width in its own column

Yes to the columns :) I believe that's the right approach. You can think of the portrait component as an element that has no idea where it's being displayed, and that's going to fill the width of its container. Then, all you have to do is define the container (columns), and append your components in it :)

Ok, should we increase the font size?

The serif on the typeface (screenshot) makes me think this is not Roboto. I think the font family declared doesn't exist (Roboto-Bold) and it fallbacks to something else depending on your system.
I'm sending you a link to the material guidelines for typography, that's usually helpful when you're not sure about which size/weight to use :)

newmuis · 2018-05-10T16:18:30Z

examples/amp-story/bookendv1.json

+    },
+    {
+      "type": "portrait",
+      "title": "This is an example portrait",


Per the other comment, this is not actually a title, it's the eyebrow. We should think about what we want to name that, and use that instead (then we can later apply it to the other components as well)

how does category sound? goes with what the mocks depict and sounds more intuitive. Also I was thinking whether we want it to be a required field.

Enriqe · 2018-05-10T21:20:01Z

@gmajoulet made the fixes you recommended. let me know what you think! here is a preview:

alanorozco · 2018-05-14T18:22:14Z

extensions/amp-story/1.0/bookend/components/heading.js

+
+  /** @override */
+  assertValidity(headingJson) {
+    if (!headingJson['text']) {


fold in assert:

rename

isValid() { return user().assert('text' in headingJson, 'Heading component must...'); }

Per @newmuis comment here I will remove the return and assert directly as you suggested.

alanorozco · 2018-05-14T18:22:40Z

extensions/amp-story/1.0/bookend/components/heading.js

+
+  /** @override */
+  build(headingJson) {
+    const heading = {


Fold in return.

alanorozco · 2018-05-14T18:23:05Z

extensions/amp-story/1.0/bookend/components/portrait.js

+   * @return {!PortraitComponentDef}
+   * */
+  build(portraitJson) {
+    const portrait = {


Fold-in return.

alanorozco · 2018-05-14T18:23:25Z

extensions/amp-story/1.0/bookend/components/portrait.js

+    addAttributesToElement(template, dict({'href': portraitData.url}));
+
+    const heading =
+    html`<h2 class="i-amphtml-story-bookend-portrait-heading"></h2>`;


alanorozco · 2018-05-14T18:23:49Z

extensions/amp-story/1.0/bookend/components/portrait.js

+    const heading =
+    html`<h2 class="i-amphtml-story-bookend-portrait-heading"></h2>`;
+    heading.textContent = portraitData.title;
+    template.appendChild(heading);


It's faster to use htmlRefs with one single template instead of building a set of smaller templates and appending to each other.

Enriqe · 2018-05-15T14:52:47Z

@alanorozco @gmajoulet PTAL

alanorozco · 2018-05-15T17:47:36Z

extensions/amp-story/1.0/bookend/components/portrait.js

@@ -79,27 +72,26 @@ export class PortraitComponent {
    const template =
        html`
        <a class="i-amphtml-story-bookend-portrait"
-          target="_top">
+          target="_top" ref="portraitComponent">
+          <h2 class="i-amphtml-story-bookend-portrait-category"


No need to prefix ref names with "portrait", it's already clear from context. You also don't need a ref (and it won't work, regardless) for the root-level element.

gmajoulet

Just a few nits.
This layout is such a big improvement on what we had before! Thanks!

gmajoulet · 2018-05-15T16:00:15Z

extensions/amp-story/1.0/amp-story-bookend.css

@@ -104,14 +115,29 @@
  padding: 0 32px !important;
 }

+.i-amphtml-story-bookend-portrait {
+  box-sizing: border-box !important;
+  display: flex;


Nit: !important

gmajoulet · 2018-05-15T16:00:36Z

extensions/amp-story/1.0/amp-story-bookend.css

+  color: inherit !important;
+  overflow: hidden !important;
+  flex: 1 0 320px !important;
+  min-width: calc(33%) !important;


You should not need the calc here

gmajoulet · 2018-05-15T16:03:29Z

extensions/amp-story/1.0/amp-story-bookend.css

 @media (min-width: 640px) {
-  .i-amphtml-story-bookend-article {
+  .i-amphtml-story-bookend-article,
+  .i-amphtml-story-bookend-portrait {
    max-width: calc(50%) !important;


I know it's not your code, but could you please remove the calc here and below too?

gmajoulet · 2018-05-15T16:04:53Z

extensions/amp-story/1.0/amp-story-bookend.css

  margin: 0 0 8px !important;
 }

+.i-amphtml-story-bookend-portrait-category {
+  margin-top: 0px !important;
+  font-family: 'Roboto' !important;


If there's no other font in this file, I think it'd be easier to set the typo on a parent div, so all the other elements will inherit from it.

gmajoulet · 2018-05-15T16:05:48Z

extensions/amp-story/1.0/amp-story-bookend.css

+
+.i-amphtml-story-bookend-portrait-image > img {
+  object-fit: cover !important;
+  position: absolute !important;


Do you still need this?

Yes, since we have to initially set the img width and height to 0px to comply with amp-img constraints and then we resize based on the available space. The position attribute makes it possible to display the image.

gmajoulet · 2018-05-15T16:07:32Z

extensions/amp-story/1.0/bookend/amp-story-bookend.js

@@ -441,11 +441,14 @@ export class AmpStoryBookend extends AMP.BaseElement {
   */
  renderComponents_(components) {
    dev().assertElement(this.bookendEl_, 'Error rendering amp-story-bookend.');
+    const container = BookendComponent


Does it make the linter happy if you do

dev().assertElement( BookendComponent.buildContainer(this.getInnerContainer_(), this.win.document));

Instead of the if (container) below?

…pdoc for getAmpdoc.

… text, mantain aspect ratio for portrait.

…ory, removes unecessary fragment.

Enriqe · 2018-05-15T20:35:37Z

thanks @alanorozco @gmajoulet! finished the changes you asked for, PTAL.

gmajoulet · 2018-05-15T20:38:12Z

extensions/amp-story/1.0/amp-story-bookend.css

  margin: 24px 0 !important;
  padding: 0 32px !important;
 }

 .i-amphtml-story-bookend-portrait {
  box-sizing: border-box !important;
-  display: flex;
+  display: flex !important;
  flex-direction: column;


!important here too : )

googlebot added the cla: yes label May 7, 2018

Enriqe force-pushed the new-bookend-components branch 4 times, most recently from c8c053e to d80f618 Compare May 9, 2018 16:25

Enriqe requested review from newmuis, gmajoulet and alanorozco May 9, 2018 16:29

newmuis reviewed May 10, 2018

View reviewed changes

Enriqe force-pushed the new-bookend-components branch from d80f618 to 2c95cbf Compare May 10, 2018 21:19

Enriqe force-pushed the new-bookend-components branch from 2c95cbf to 1209eb2 Compare May 11, 2018 19:09

Enriqe mentioned this pull request May 14, 2018

Bookend articles URLs can break the layout #14248

Closed

alanorozco requested changes May 14, 2018

View reviewed changes

alanorozco requested changes May 15, 2018

View reviewed changes

gmajoulet reviewed May 15, 2018

View reviewed changes

Enriqe added 8 commits May 15, 2018 16:32

Adds service registers, removes buildCallback, and changes this.getAm…

9a35c23

…pdoc for getAmpdoc.

Adds portrait component. Changes article-set-title name to heading.

f2a9a9e

change iamge to flexbox.

ef3a7a7

Fix test for heading.

ef5d7d8

move uppercase logic to css, center images, use ellipsis for wrapping…

064b65e

… text, mantain aspect ratio for portrait.

change small article titles to hide after 3 lines using line height.

297b7ad

uses hrefs instead of multiple templates, changes title name to categ…

3eff9a3

…ory, removes unecessary fragment.

adds typedef for htmlRefs deconstruction

2ac4129

jsdoc, removes unecessary calcs in css, inherit font type.

75143d4

Enriqe force-pushed the new-bookend-components branch from 8eb300f to 75143d4 Compare May 15, 2018 20:32

gmajoulet approved these changes May 15, 2018

View reviewed changes

alanorozco approved these changes May 15, 2018

View reviewed changes

missing importants in css

d7bced6

Enriqe force-pushed the new-bookend-components branch from d29b538 to d7bced6 Compare May 15, 2018 21:00

fix jsdoc warnings.

2d3d2ce

gmajoulet merged commit 11f3e23 into ampproject:master May 16, 2018

Enriqe deleted the new-bookend-components branch May 16, 2018 14:27

Enriqe mentioned this pull request May 16, 2018

Rename "article-set-title" bookend component to "heading" #15088

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ New amp-story-bookend components: portrait #15142

✨ New amp-story-bookend components: portrait #15142

Enriqe commented May 7, 2018 •

edited

gmajoulet commented May 10, 2018

Enriqe commented May 10, 2018 •

edited

newmuis commented May 10, 2018 •

edited

Enriqe commented May 10, 2018

gmajoulet commented May 10, 2018

newmuis May 10, 2018

Enriqe May 10, 2018 •

edited

Enriqe commented May 10, 2018

alanorozco May 14, 2018

Enriqe May 15, 2018

alanorozco May 14, 2018

alanorozco May 14, 2018

alanorozco May 14, 2018

alanorozco May 14, 2018

Enriqe commented May 15, 2018

alanorozco May 15, 2018

gmajoulet left a comment

gmajoulet May 15, 2018

gmajoulet May 15, 2018

gmajoulet May 15, 2018

gmajoulet May 15, 2018

gmajoulet May 15, 2018

Enriqe May 15, 2018

gmajoulet May 15, 2018

Enriqe commented May 15, 2018

gmajoulet May 15, 2018

Enriqe May 15, 2018

✨ New amp-story-bookend components: portrait #15142

✨ New amp-story-bookend components: portrait #15142

Conversation

Enriqe commented May 7, 2018 • edited

gmajoulet commented May 10, 2018

Enriqe commented May 10, 2018 • edited

newmuis commented May 10, 2018 • edited

Enriqe commented May 10, 2018

gmajoulet commented May 10, 2018

Choose a reason for hiding this comment

Enriqe May 10, 2018 • edited

Choose a reason for hiding this comment

Enriqe commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enriqe commented May 15, 2018

Choose a reason for hiding this comment

gmajoulet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enriqe commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enriqe commented May 7, 2018 •

edited

Enriqe commented May 10, 2018 •

edited

newmuis commented May 10, 2018 •

edited

Enriqe May 10, 2018 •

edited