AMP Sidebar V1: Skeleton'd the files for the feature #9784
Conversation
torch2424
commented
Jun 7, 2017
torch2424
commented
- Added required files
- Added gulp file rules for the new version, and alias for v0.1
This skeeletons all of the required files for amp-sidebar v1
Now the gulp file will build and look for v1.0 of sidebar
|
@torch2424 may want to update the copyright dates on the files to 2017 |
Also, tested to ensure that the extension could be loaded and registered successfully
|
@honeybadgerdontcare Oh yes of course, my apologies. |
Suggested by @honeybadgerdontcare
This is because of: ampproject#9642 , Was going to fix this myself, but it is currently in PR at: ampproject#9730
build-system/tasks/dep-check.js
Outdated
| @@ -203,7 +203,9 @@ function getGraph(entryModule) { | |||
| */ | |||
| function getEntryModule(extensionFolder) { | |||
| // TODO (@zhouyx, #9642): Remove the special check and handle more than just 0.1 | |||
| if (extensionFolder == 'extensions/amp-sticky-ad') { | |||
| if (extensionFolder == 'extensions/amp-sticky-ad' || | |||
There was a problem hiding this comment.
You can't do this for amp-sidebar. amp-sticky-ad is OK because 0.1 is fully deprecated.
There was a problem hiding this comment.
Ahhhh that makes sense! Thank you!
gulpfile.js
Outdated
| @@ -138,6 +138,8 @@ declareExtension('amp-viewer-integration', '0.1', { | |||
| declareExtension('amp-video', '0.1', false); | |||
| declareExtension('amp-youtube', '0.1', false); | |||
| declareExtensionVersionAlias( | |||
| 'amp-sidebar', '0.1', /* lastestVersion */ '1.0', /* hasCss */ true); | |||
There was a problem hiding this comment.
this is not ok. It basic copy minimized 1.0 script to 0.1 version. Should only be added after 0.1 version is fully deprecated.
There was a problem hiding this comment.
Okay awesome, I know things looked kinda fishy in that function, thank you!
Was my error in not realizing alias'ing was done for deprecated extensions
There was a problem hiding this comment.
Few more requests.
Regarding the dep-check.js check, since you revert the change, dep-check for side-bar 1.0 does not exist. I am OK with it now since there's no actual implementation there, also #9730 will eventually fix the issue.
Another thing is I realize declareExtensionVersionAlias can be very dangerous to use. It will break an extension version, so it would be great if you can add a comment in this PR.
| } | ||
| } | ||
|
|
||
| AMP.registerElement('amp-sidebar', AmpSidebar, CSS); |
There was a problem hiding this comment.
declare version '1.0' here for future testing purpose.
https://github.com/ampproject/amphtml/blob/master/extensions/amp-sticky-ad/1.0/amp-sticky-ad.js#L256
There was a problem hiding this comment.
Ah yes, I saw that in AMP sticky ad, and removed it as I didn't know if it was necessary, thank you!
|
LGTM |
| @@ -168,6 +168,10 @@ function declareExtension(name, version, hasCssOrOptions, opt_noTypeCheck, | |||
| } | |||
|
|
|||
| /** | |||
| * This function is used for declaring deprecated extensions. It simply places the current | |||
gulpfile.js
Outdated
| * This function is used for declaring deprecated extensions. It simply places the current | ||
| * version code in place of the latest versions. | ||
| * This has the ability to break an extension verison, so please be sure that this is | ||
| * the correct to use. |
There was a problem hiding this comment.
nit: "the correct one to use"?
| @@ -168,6 +168,10 @@ function declareExtension(name, version, hasCssOrOptions, opt_noTypeCheck, | |||
| } | |||
|
|
|||
| /** | |||
| * This function is used for declaring deprecated extensions. It simply places the current | |||
Needed a different way of building the window since we are now introducing versioning to amp-sidebar
|
These validator changes should be live everywhere within an hour. |
