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
[amp-sidebar 1.0] Un-version sidebar 1.0 #10796
Conversation
@@ -15,13 +15,19 @@ | |||
*/ | |||
|
|||
import {CSS} from '../../../build/amp-sidebar-0.1.css'; | |||
import {Layout} from '../../../src/layout'; | |||
import {dev, user} from '../../../src/log'; | |||
import {isExperimentOn} from '../../../src/experiments'; | |||
import {KeyCodes} from '../../../src/utils/key-codes'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move K between C & L
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
toArray(this.element.querySelectorAll('nav[toolbar]')); | ||
|
||
if (toolbarElements.length > 0) { | ||
user().assert(isExperimentOn(this.win, TAG), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the experiment check above - don't assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
/** @override */ | ||
onLayoutMeasure() { | ||
// Check our toolbars for changes | ||
this.toolbars_.forEach(toolbar => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
experiment guard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
</ul> | ||
</amp-sidebar> | ||
``` | ||
|
||
*Example: Advanced usage of `amp-sidebar` with anchor links and smooth scrolling* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not advanced usage - make this a part of the default example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -54,6 +55,52 @@ describes.realWin('amp-sidebar 0.1 version', { | |||
const anchor = iframe.doc.createElement('a'); | |||
anchor.href = '#section1'; | |||
ampSidebar.appendChild(anchor); | |||
if (options.toolbars) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create a separate describe for tests with toolbar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return Promise.resolve(); | ||
}); | ||
// Create our individual toolbars | ||
options.toolbars.forEach(toolbarObj => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this to its own function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -104,6 +117,89 @@ Example: | |||
<button on='tap:sidebar1.close'>x</button> | |||
``` | |||
|
|||
### Toolbar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mark it as experimental (Experimental)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
closes #10770
By request of the validation team, and approval by @camelburrito and @dvoytenko , this PR will unversion sidebar.
toolbar
will now live as a feature toamp-sidebar
, and is hidden under the experimental flag,amp-sidebar toolbar
. This PR does the following changes:History of toolbar.js to ensure it has not changed since PR was opened: https://github.com/ampproject/amphtml/commits/master/extensions/amp-sidebar/1.0/toolbar.js
See the screenshots below showing it will not affect current users, but only experimental toolbar users: