Sidebar - Basic Open and close functionality #2795
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
camelburrito
force-pushed
the
sidebar__prs_2
branch
from
3f0f2ac to
a959356
Compare
|
CLAs look good, thanks! |
camelburrito
force-pushed
the
sidebar__prs_2
branch
from
a959356 to
8525d49
Compare
camelburrito
force-pushed
the
sidebar__prs_2
branch
from
8525d49 to
461e2af
Compare
| @@ -14,4 +14,48 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| amp-sidebar {} | |||
| amp-sidebar { | |||
| position: fixed !important; | |||
There was a problem hiding this comment.
Didn't we decide to remove !important here?
There was a problem hiding this comment.
removed on the top property
|
@sriramkrish85 Pretty much at LGTM with just a few comments. |
camelburrito
force-pushed
the
sidebar__prs_2
branch
2 times, most recently
from
1c5b845 to
b0a8769
Compare
|
PTAL |
| this.viewport_.disableTouchZoom(); | ||
| this.mutateElement(() => { | ||
| this.viewport_.addToFixedLayer(this.element); | ||
| setStyles(this.element, { |
There was a problem hiding this comment.
Please use toggle from style.js. Here and everywhere where display is modified. Note that it doesn't set "block" - it only removes "none", which is actually the correct behavior in most of cases, but something to keep in mind.
There was a problem hiding this comment.
in this case the sidebar needs to be block; since it is a custom element. we would need to make it block
|
LGTM with one group of fixes requested with |
camelburrito
force-pushed
the
sidebar__prs_2
branch
3 times, most recently
from
2d60b0a to
99634b1
Compare
No description provided.