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

create 1.0 version for amp-sticky-ad extension #5923

Merged
merged 5 commits into from Nov 2, 2016

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented Oct 31, 2016

#5921
Validator changes not included.
Actual changes to V1 version not included .

@zhouyx
Copy link
Contributor Author

zhouyx commented Oct 31, 2016

I am having test failure withe message

Chrome 54.0.2840 (Mac OS X 10.11.6) ERROR
Uncaught Error: amp-sticky-ad is already registered. The script tag for amp-sticky-ad is likely included twice in the page.​​​

I have already adopted the describes framework but that doesn't work for me. What's a good way to test different version of one same amp extension? @dvoytenko?

@zhouyx
Copy link
Contributor Author

zhouyx commented Nov 2, 2016

Thanks to @dvoytenko, finally fixed the tests. @erwinmombay PTAL

* limitations under the License.
*/

import {CSS} from '../../../build/amp-sticky-ad-1.0.css';
Copy link
Member

Choose a reason for hiding this comment

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

qq beside version change there isn't actual any change yet to the code right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope. I feel it would be too much for one PR, and hard to diff the changes in this PR. so i simply copied the code.

Copy link
Member

Choose a reason for hiding this comment

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

yeah thats good. LGTM

@zhouyx zhouyx merged commit d5bcdc4 into ampproject:master Nov 2, 2016
mrsufgi pushed a commit to apester-dev/amphtml that referenced this pull request Nov 2, 2016
* add 1.0 folder

* add test

* test fixed by Dima

* fix tests

* fix share-tracking test

added html_format: AMP @powdercloud

new line in test-amp-apester-media

validator fix #2

removed decodeUrl

validator fix: data-apester-media to data-apester-media-id

minor changes to css and loader

refactor(css): change css classes

css
mrsufgi pushed a commit to apester-dev/amphtml that referenced this pull request Nov 2, 2016
* add 1.0 folder

* add test

* test fixed by Dima

* fix tests

* fix share-tracking test

added html_format: AMP @powdercloud

new line in test-amp-apester-media

validator fix #2

removed decodeUrl

validator fix: data-apester-media to data-apester-media-id

minor changes to css and loader

refactor(css): change css classes

css

added html_format: AMP @powdercloud

new line in test-amp-apester-media

validator fix #2

removed decodeUrl

validator fix: data-apester-media to data-apester-media-id

refactor(css): change css classes

css
@zhouyx zhouyx deleted the sticky-ad-1.0 branch November 3, 2016 00:04
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
* add 1.0 folder

* add test

* test fixed by Dima

* fix tests

* fix share-tracking test
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
* add 1.0 folder

* add test

* test fixed by Dima

* fix tests

* fix share-tracking test
@nbeloglazov
Copy link
Contributor

I'm curious, why amp-sticky-ad was bumped to 1.0 while all other extensions are 0.1? The reason is that there are some example pages in AMP repo that still reference 0.1.

@zhouyx
Copy link
Contributor Author

zhouyx commented Apr 16, 2019

We try not to bump up component version because of the painful deprecation process. However we did this to amp-sticky-ad because we decided to change the default UI of the component after UX study, and cannot avoid a breaking change without bumping up version.

The reason is that there are some example pages in AMP repo that still reference 0.1.

Thank you for reporting this. Could you please let me know? This isn't an issue because all 0.1 script point to 1.0 script today. But it's definitely best that we fix them.

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.

None yet

4 participants