New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement new version of ads placeholder #8261

Closed
jasti opened this Issue Mar 20, 2017 · 9 comments

Comments

@jasti
Collaborator

jasti commented Mar 20, 2017

Once @spacedino's done with #8238 in the next sprint - implement the changes.

@jasti jasti added the Category: Ads label Mar 20, 2017

@jasti jasti added this to the Sprint H1 April milestone Mar 20, 2017

@jasti jasti changed the title from Implement new version of ads placehoder to Implement new version of ads placeholder Mar 20, 2017

@jasti jasti modified the milestones: Sprint H2 April, Sprint H1 April Apr 3, 2017

@zhouyx

This comment has been minimized.

Show comment
Hide comment
@zhouyx

zhouyx Apr 17, 2017

Collaborator

The new placeholder is alive in canary. Please goto https://cdn.ampproject.org/experiments.html opt into AMP dev channel, and opt into either 'ad-loader-v1' or 'ad-loader-v2' to see the new design.

Collaborator

zhouyx commented Apr 17, 2017

The new placeholder is alive in canary. Please goto https://cdn.ampproject.org/experiments.html opt into AMP dev channel, and opt into either 'ad-loader-v1' or 'ad-loader-v2' to see the new design.

@spacedino

This comment has been minimized.

Show comment
Hide comment
@spacedino

spacedino Apr 17, 2017

@zhouyx is this the way the publisher implemented or is this us? the loader is above the placeholder text "advertiser"

@zhouyx is this the way the publisher implemented or is this us? the loader is above the placeholder text "advertiser"

@zhouyx

This comment has been minimized.

Show comment
Hide comment
@zhouyx

zhouyx Apr 18, 2017

Collaborator

The text "advertiser" is implemented by publisher

Collaborator

zhouyx commented Apr 18, 2017

The text "advertiser" is implemented by publisher

@jasti jasti reopened this Apr 19, 2017

@jasti

This comment has been minimized.

Show comment
Hide comment
@jasti

jasti Apr 19, 2017

Collaborator

@zhouyx The implementation doesn't match the mocks. Specifically, the corner angles on the ad logo are more acute than in the mocks.
See original
original

See Implementation
screen shot 2017-04-19 at 2 37 17 pm

CC @spacedino

Collaborator

jasti commented Apr 19, 2017

@zhouyx The implementation doesn't match the mocks. Specifically, the corner angles on the ad logo are more acute than in the mocks.
See original
original

See Implementation
screen shot 2017-04-19 at 2 37 17 pm

CC @spacedino

@zhouyx

This comment has been minimized.

Show comment
Hide comment
@zhouyx

zhouyx Apr 19, 2017

Collaborator

I think @spacedino and I adjust the border-radius a little bit. I am very happy to adjust it.

Collaborator

zhouyx commented Apr 19, 2017

I think @spacedino and I adjust the border-radius a little bit. I am very happy to adjust it.

@jasti

This comment has been minimized.

Show comment
Hide comment
@jasti

jasti Apr 19, 2017

Collaborator

@spacedino reasoning behind the change?

Collaborator

jasti commented Apr 19, 2017

@spacedino reasoning behind the change?

@zhouyx

This comment has been minimized.

Show comment
Hide comment
@zhouyx

zhouyx Apr 25, 2017

Collaborator

New design (Note: size may appear different due to different resolution):
image
The text is closer to top in Nexus 6p. Can't center the text perfectly unless using svg : (
Or please go to link to see placeholder in real device.

Collaborator

zhouyx commented Apr 25, 2017

New design (Note: size may appear different due to different resolution):
image
The text is closer to top in Nexus 6p. Can't center the text perfectly unless using svg : (
Or please go to link to see placeholder in real device.

@zhouyx

This comment has been minimized.

Show comment
Hide comment
@zhouyx

zhouyx Apr 28, 2017

Collaborator
Collaborator

zhouyx commented Apr 28, 2017

@zhouyx

This comment has been minimized.

Show comment
Hide comment
@zhouyx

zhouyx Apr 28, 2017

Collaborator

Close this issue for now. Will wait until the change hit canary and we choose which design to go with later.

Collaborator

zhouyx commented Apr 28, 2017

Close this issue for now. Will wait until the change hit canary and we choose which design to go with later.

@zhouyx zhouyx closed this Apr 28, 2017

@jasti jasti added this to Done in AMP Advertising Jun 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment