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.

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

Update csa.md to remove non-required parameters #6902

Merged
merged 2 commits into from
Jan 14, 2017

Conversation

mmcao
Copy link
Contributor

@mmcao mmcao commented Jan 5, 2017

Removed non-required parameters (width, layout) from the amp-ad tag.

Removed non-required parameters (width, layout) from the amp-ad tag.
@mmcao
Copy link
Contributor Author

mmcao commented Jan 5, 2017

@megankj can you review?

Copy link

@megankj-zz megankj-zz left a comment

Choose a reason for hiding this comment

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

LGTM for removing the unnecessary attributes. But there were some instances where it looks like the options could fit on one line. Can you fix that as well?

type='csa'
layout='fixed-height'
data-afsh-page-options='{"pubId": "partner-vert-pla-pubid-pdp",
"query": "flowers"}'

Choose a reason for hiding this comment

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

can this fit on one line?

type='csa'
layout='fixed-height'
data-afsh-page-options='{"pubId": "partner-vert-pla-pubid-pdp",
"query": "flowers"}'

Choose a reason for hiding this comment

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

can this fit on one line?

@jridgewell
Copy link
Contributor

/to @bpaduch

@megankj-zz
Copy link

Can this PR be merged?

@lannka lannka merged commit 01bb681 into ampproject:master Jan 14, 2017
rpominov pushed a commit to yandex-pcode/amphtml that referenced this pull request Jan 20, 2017
* master: (310 commits)
  Update csa.md to remove non-required parameters (ampproject#6902)
  Add notes about requesting ads ATF and link to demo (ampproject#7037)
  Remove whitelist for lightbox scrollable validator (ampproject#7034)
  Delegate submit events until amp-form is loaded  (ampproject#6929)
  Moves closure sha384 into a new extension amp-crypto-polyfill for lazy load (ampproject#7006)
  Refactor observables in viewer-impl into a map object (ampproject#7004)
  resizing of margins (ampproject#6824)
  Use URL replacer from embed for pixel (ampproject#7029)
  adds support for Gemius analytics (ampproject#6558)
  Avoid duplicating server-layout (ampproject#7021)
  Laterpay validator config (ampproject#6974)
  Validator rollup (ampproject#7023)
  skeleton for amp-tabs (ampproject#7003)
  Upgrade post-css and related packages to latest (ampproject#7020)
  handle unload (ampproject#7001)
  viewer-integr.js -> amp-viewer-integration (ampproject#6989)
  dev().info()->dev().fine() (ampproject#7017)
  Turned on experiment flag (ampproject#6774)
  Unlaunch ios-embed-wrapper for iOS8 to avoid scroll freezing issues (ampproject#7018)
  Add some A4A ad request parameters (ampproject#6643)
  ...
jridgewell pushed a commit to jridgewell/amphtml that referenced this pull request Jan 31, 2017
* Update csa.md to remove non-required parameters 

Removed non-required parameters (width, layout) from the amp-ad tag.

* Updated csa.md to combine text in multiple lines
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
* Update csa.md to remove non-required parameters 

Removed non-required parameters (width, layout) from the amp-ad tag.

* Updated csa.md to combine text in multiple lines
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants