Skip to content

Conversation

calebcordry
Copy link
Member

image

Auto progresses ad and bar shows 8 second animation. Will be ran as an experiment to determine UX improvements.

Partial #33969

@amp-owners-bot
Copy link

amp-owners-bot bot commented Jun 3, 2021

Hey @gmajoulet, @newmuis! These files were changed:

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

@calebcordry
Copy link
Member Author

cc/ @jshamble

@newmuis
Copy link
Contributor

newmuis commented Jun 3, 2021

This is cool!

Sorry, I forget a lot of the ads context: are ads ever removed and the user progresses through the story? Put another way: is it certain that when navigating backwards, the user will still see all of the ads that they previously saw, in the same slots, in reverse order?

@gmajoulet
Copy link
Contributor

If I understand your concern correctly Jon: this does not add a segment, but colors the current active one in yellow instead of white, so back navigation should not be impacted.

@calebcordry
Copy link
Member Author

calebcordry commented Jun 3, 2021

@newmuis yep that is right, once an ad is place navigating backwards will show the same ad in the same position.

but colors the current active one in yellow instead of white

Right, this PR selects the segment based on the index from the page change event and creates a yellow div on top. I am open to other ideas if you all have them.

We may try adding a new segment at some point but I think it will be much more complicated, and still needs UX to answer questions like what happens on no-fill? Also with our current placement algo we don't know positions in advance but this will likely change soon with the introduction of our new placement logic.

@calebcordry
Copy link
Member Author

@gmajoulet friendly ping :)

Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider an alternative implementation where the progress bar class listens for AD_STATE updates and renders that extra segment itself? The code would be more contained

@calebcordry
Copy link
Member Author

Did you consider an alternative implementation where the progress bar class listens for AD_STATE updates and renders that extra segment itself? The code would be more contained

I thought it might be better to minimize the ad specific code in the extension, but I don't feel strongly. If you think its better I can look into moving it there. It may be more difficult to run various experiments i.e. control group should not contain stories where there is no amp-story-auto-ads but I think we could make it work.

@calebcordry
Copy link
Member Author

Closing in favor of #34804

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants