amp-sidebar #2461
amp-sidebar #2461
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
branch
11 times, most recently
from
3571d70 to
1ac272c
Compare
|
I signed it! |
| <table> | ||
| <tr> | ||
| <td width="40%"><strong>Description</strong></td> | ||
| <td>A sidebar provides a way to display meta content intended for temporary access (navigation buttons, chat contact list, etc.),.The sidebar can be revealed by a button tap while the main content remains visually underneath.</td> |
There was a problem hiding this comment.
nit: Comma between these two sentences should be removed.
camelburrito
force-pushed
the
sidebar
branch
3 times, most recently
from
2dba451 to
1dc7c93
Compare
| .amp-sidebar-closed amp-sidebar, | ||
| .amp-sidebar-open amp-sidebar { | ||
| transition: transform 0.5s ease; | ||
| will-change: transform |
There was a problem hiding this comment.
you might actually want to add will-change to the default state not on the transition state since you want to give the engine a heads up. (this might be too late).
There was a problem hiding this comment.
possibly the most optimal place to actually add this is on your click handlers. where open will add it, then close will remove it. the mutate handler callback gives it enough time to get optimized.
There was a problem hiding this comment.
Dropping will-change for now - will consider adding it if the performance is not very good
| /** @private @const {boolean} */ | ||
| this.hasMask_ = false; | ||
|
|
||
| if (this.direction_ != 'left' && this.direction_ != 'right') { |
There was a problem hiding this comment.
Are we supporting dir="rtl" here as well? It could be a source of the default at the very least.
There was a problem hiding this comment.
Yes - changed to use dir and inherit dir from the body
| return; | ||
| } | ||
| const mask = this.document_.createElement('div'); | ||
| mask.setAttribute('class', '-amp-sidebar-mask'); |
There was a problem hiding this comment.
Let's still use classList.add() here.
| */ | ||
| fixIosElasticScrollLeak_() { | ||
| this.element.addEventListener('scroll', e => { | ||
| this.mutateElement(() => { |
There was a problem hiding this comment.
This shouldn't go in the mutateElement, unfortunately. Maybe vsync? Or maybe even direct since this is a hack. But mutate element will increase risk condition too much. So will vsync, so let's start with them stripped and let's see what profiler says.
There was a problem hiding this comment.
It seems to work well in mutateElement and the direct approach. removing the mutate now.
camelburrito
force-pushed
the
sidebar
branch
8 times, most recently
from
7a8f159 to
fd240ab
Compare
- Fix for Ios elastic scrolling - Fix for viewer top padding. - Disable touch zoom. - Review comment fixes. - Support for dir instead of direction. - Remove vsync and add a padding fix by creating dummy elements. - Add more tests. - linting. Disable zoom on sidebar open
…ayout type to no-display and add aria support.
#2302