✨ Show label when live story is updated and user is on last page. #22700

Enriqe · 2019-06-05T17:53:30Z

Follow up for #22519
Partial for #21714

Shows a label when a live story is updated with new pages and the user is on the last page.

Preview:

EDIT: updated UX

extensions/amp-story/1.0/_locales/en.js

extensions/amp-story/1.0/amp-story-system-layer.js

lgtm-com · 2019-06-05T18:32:21Z

This pull request introduces 1 alert when merging e009887 into 7d9c822 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

Enriqe · 2019-06-10T20:35:23Z

Updated the PR with the most recent UX spec. PTAL.

See previews below for reference:

You'll notice that on the three panel view, the pagination button isn't updated when there is a new page. This is tracked here #22770

newmuis · 2019-06-10T21:08:47Z

Actual	Expected

Some quick minor UI nits:

Can't tell if it's just because of the low fidelity of the GIF, but it looks like there's only one circle and not two?
The font should be bold
The mocks look like the corners are not completely sharp; @hongwei1990 can you verify?
It looks like there's too much space on the left and/or too little space on the right

Note that I haven't actually taken a look at the code yet; this is all based on the rendering in the GIF

Enriqe · 2019-06-10T21:36:51Z

Can't tell if it's just because of the low fidelity of the GIF, but it looks like there's only one circle and not two?

Yep, it's the gif, sorry. Here is a screenshot with a better resolution of how it actually looks. You can see that the font is already bold.

I corrected the opacity of the outer circle to the spec and now it's clearer that there are two circles. I also added a border radius for the box edges but I will increase it to make it look more like the mocks since I didn't get an exact number.

Also added a bit more space on the right.

gmajoulet · 2019-06-10T21:42:19Z

Could you please post a screenshot of that UI when displayed RTL? The last margin you added might break that display :(

On Mon, Jun 10, 2019 at 5:37 PM Enrique Marroquin ***@***.***> wrote: Can't tell if it's just because of the low fidelity of the GIF, but it looks like there's only one circle and not two? Yep, it's the gif, sorry. Here is a screenshot with a better resolution of how it actually looks. You can see that the font is already bold. I corrected the opacity of the outer circle to the spec and now it's clearer that there are two circles. I also added a border radius for the box edges but I will increase it to make it look more like the mocks since I didn't get an exact number. Also added a bit more space on the right. [image: image] <https://user-images.githubusercontent.com/5449100/59228523-0ec2f300-8ba6-11e9-80c3-dc992943cfef.png> — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#22700?email_source=notifications&email_token=AALMITDZH6UCF3XFO2ENJQLPZ3CRLA5CNFSM4HUBN3KKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXLJ5FI#issuecomment-500604565>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALMITGEHY45TRXV7FRLDXDPZ3CRLANCNFSM4HUBN3KA> .

-- Gabriel Majoulet | UX Engineer | gmajoulet@google.com

newmuis · 2019-06-10T21:40:58Z

extensions/amp-story/1.0/amp-story-system-layer.js

    if (!this.isBuilt_) {
      return;
    }
    this.vsync_.mutate(() => {
-      this.getShadowRoot().setAttribute(MESSAGE_DISPLAY_CLASS, 'noshow');
+      this.getShadowRoot().setAttribute(message, 'noshow');


Sorry I know this is the type of thing that's very difficult to test (manually, or in any automated fashion), but how do multiple messages override each other? Does the first one just disgust completely once the second one comes in?

I suspect that would be the case, but I'm not actually sure whether that's okay? This can result in weird edge cases when one message is triggered right after another. It's probably fine if it's two audio message since they'd both come from user interactions, but if it's an audio message followed by a live list update, the user won't have had time to read the audio message.

Should we consider queuing the messages like toast frameworks (likely) would?

I tested it and right now they are both shown, and eventually disappear. Since this seems to be a very specific edge case, do you mind if I address it in a follow-up?

I filed #22778 to keep track.

extensions/amp-story/1.0/amp-story-system-layer.js

newmuis · 2019-06-10T21:48:38Z

extensions/amp-story/1.0/amp-story-system-layer.css

+
+.i-amphtml-story-has-new-page-circle-icon {
+	background: rgba(3, 255, 160, 1) !important;
+	border-radius: 50% !important;


This should have a 4px border at rgba(3, 255, 160, 0.5)

Did you mean to comment this on the line below of the box-shadow, which represents the outer circle?

If so, according to the spec the outer circle is supposed to be 10px in width, while the inner circle is 6px in width.

And if I understand correctly, the box-shadow spread property specifies the radius of the circle. Meaning that if we want a 10px-wide circle, then the want to have a 5px radius. And since the inner circle is a 6px-wide circle (with a radius of 3px), then the box-shadow spread property should be 2px to add up to a total radius of 5px, giving us a 10px-wide outer circle.

Enriqe

Could you please post a screenshot of that UI when displayed RTL?

Thanks for bringing it up, I had screwed up RTL with my latest commit. I fixed it now. Here you can see the latest screenshots:

LTR:

RTL:

extensions/amp-story/1.0/amp-story-system-layer.js

Enriqe · 2019-06-11T21:29:19Z

This is ready for another review 👍

newmuis

Would be good to add a visual test for this; could come as a separate PR

extensions/amp-story/1.0/amp-story-system-layer.css

Enriqe · 2019-06-11T22:47:20Z

Would be good to add a visual test for this; could come as a separate PR

Sounds good, will do in a follow up PR. Thanks both!

…pproject#22700) * show label when story is updated and user is on last page. * better translation label & use same timeoutid. * revised position and style of label according to newest UX spec. * add more space on the right of the text and increase border radius. * fix padding and circle color values. * fix rtl, add i-amphtml prefix * fix css

Enriqe added the WG: stories label Jun 5, 2019

Enriqe self-assigned this Jun 5, 2019

googlebot added the cla: yes label Jun 5, 2019

Enriqe requested review from gmajoulet and newmuis June 5, 2019 17:53

This was referenced Jun 5, 2019

Fills navigation path with skipped pages #22599

Closed

[Master feature] Support live AMP Stories #21714

Closed

gmajoulet reviewed Jun 5, 2019

View reviewed changes

extensions/amp-story/1.0/_locales/en.js Outdated Show resolved Hide resolved

extensions/amp-story/1.0/amp-story-system-layer.js Show resolved Hide resolved

extensions/amp-story/1.0/amp-story-system-layer.js Outdated Show resolved Hide resolved

Enriqe added 3 commits June 10, 2019 16:32

show label when story is updated and user is on last page.

f070f6f

better translation label & use same timeoutid.

21fc44e

revised position and style of label according to newest UX spec.

456dafc

Enriqe force-pushed the story-updated-label branch from 5b7af93 to 456dafc Compare June 10, 2019 20:32

add more space on the right of the text and increase border radius.

d7686c7

Enriqe force-pushed the story-updated-label branch from b8c5f1d to d7686c7 Compare June 10, 2019 21:26

fix padding and circle color values.

e2fc00d

Enriqe force-pushed the story-updated-label branch from 0af6d25 to e2fc00d Compare June 10, 2019 21:34

newmuis reviewed Jun 10, 2019

View reviewed changes

fix rtl, add i-amphtml prefix

d3030a1

Enriqe commented Jun 11, 2019

View reviewed changes

extensions/amp-story/1.0/amp-story-system-layer.js Show resolved Hide resolved

newmuis approved these changes Jun 11, 2019

View reviewed changes

gmajoulet approved these changes Jun 11, 2019

View reviewed changes

extensions/amp-story/1.0/amp-story-system-layer.css Outdated Show resolved Hide resolved

extensions/amp-story/1.0/amp-story-system-layer.css Outdated Show resolved Hide resolved

fix css

860af88

Enriqe merged commit 53351cb into ampproject:master Jun 12, 2019

Enriqe deleted the story-updated-label branch June 12, 2019 21:03

newmuis added this to Done in wg-stories Live Stories Sep 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Show label when live story is updated and user is on last page. #22700

✨ Show label when live story is updated and user is on last page. #22700

Enriqe commented Jun 5, 2019 •

edited

lgtm-com bot commented Jun 5, 2019

Enriqe commented Jun 10, 2019

newmuis commented Jun 10, 2019

Enriqe commented Jun 10, 2019

gmajoulet commented Jun 10, 2019 via email

newmuis Jun 10, 2019

Enriqe Jun 11, 2019

newmuis Jun 10, 2019

Enriqe Jun 11, 2019

Enriqe Jun 11, 2019

Enriqe left a comment •

edited

Enriqe commented Jun 11, 2019

newmuis left a comment

Enriqe commented Jun 11, 2019 •

edited

✨ Show label when live story is updated and user is on last page. #22700

✨ Show label when live story is updated and user is on last page. #22700

Conversation

Enriqe commented Jun 5, 2019 • edited

lgtm-com bot commented Jun 5, 2019

Enriqe commented Jun 10, 2019

newmuis commented Jun 10, 2019

Enriqe commented Jun 10, 2019

gmajoulet commented Jun 10, 2019 via email

newmuis Jun 10, 2019

Choose a reason for hiding this comment

Enriqe Jun 11, 2019

Choose a reason for hiding this comment

newmuis Jun 10, 2019

Choose a reason for hiding this comment

Enriqe Jun 11, 2019

Choose a reason for hiding this comment

Enriqe Jun 11, 2019

Choose a reason for hiding this comment

Enriqe left a comment • edited

Choose a reason for hiding this comment

Enriqe commented Jun 11, 2019

newmuis left a comment

Choose a reason for hiding this comment

Enriqe commented Jun 11, 2019 • edited

Enriqe commented Jun 5, 2019 •

edited

Enriqe left a comment •

edited

Enriqe commented Jun 11, 2019 •

edited