Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update ad loading UX. #7167

Merged
merged 2 commits into from
Jan 24, 2017
Merged

Update ad loading UX. #7167

merged 2 commits into from
Jan 24, 2017

Conversation

lannka
Copy link
Contributor

@lannka lannka commented Jan 23, 2017 •
edited

  • Add opacity to ad placeholder background so that it's visible in both white and dark pages.
  • Use CSS :after selector to display the ad logo, so that is easily overriddable by pub for i18n.
  • Fix a CSS bug caused by hard coded width/height of the ad logo.

For #5918

ezgif com-video-to-gif

Contrast under different background colors:

image

image

image

image

@lannka lannka requested a review from dvoytenko January 23, 2017 23:28
@spacedino
Copy link

lgtm. has @jasti seen it?

@jasti
Copy link
Contributor

jasti commented Jan 24, 2017 •
edited

LGTM.

dvoytenko
dvoytenko suggested changes Jan 24, 2017
View reviewed changes
extensions/amp-ad/0.1/amp-ad-ui.js
`<div class='-amp-ad-default-holder'>
<div class='-amp-ad-tag'>Ad</div>
</div>`;
const HOLDER_HTML = `<div class='-amp-ad-default-holder'></div>`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please redo as a direct DOM construction. innerHTML is not allowed for cases like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@lannka
Copy link
Contributor Author

lannka commented Jan 24, 2017

PTAL

@lannka lannka merged commit 324d352 into ampproject:master Jan 24, 2017
@lannka lannka deleted the ad-loading-ux branch January 24, 2017 19:39
jridgewell pushed a commit to jridgewell/amphtml that referenced this pull request Jan 31, 2017
@lannka @jridgewell
Update ad loading UX. (ampproject#7167)
835b138 
* Update ad loading UX.

* Address comment.
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
@lannka @mrjoro
Update ad loading UX. (ampproject#7167)
5b1b705 
* Update ad loading UX.

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

@dvoytenko dvoytenko dvoytenko approved these changes

Assignees

@dvoytenko dvoytenko

@spacedino spacedino

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lannka @spacedino @jasti @dvoytenko