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 #2461

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@camelburrito
Copy link
Collaborator

camelburrito commented Mar 5, 2016

@googlebot

This comment has been minimized.

Copy link

googlebot commented Mar 5, 2016

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. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@camelburrito camelburrito force-pushed the camelburrito:sidebar branch 11 times, most recently Mar 5, 2016

@camelburrito

This comment has been minimized.

Copy link
Collaborator

camelburrito commented Mar 6, 2016

I signed it!

@rudygalfi

View changes

extensions/amp-sidebar/amp-sidebar.md Outdated
<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>

This comment has been minimized.

@rudygalfi

rudygalfi Mar 6, 2016

Contributor

nit: Comma between these two sentences should be removed.

This comment has been minimized.

@camelburrito

camelburrito Mar 29, 2016

Collaborator

Done

@camelburrito camelburrito force-pushed the camelburrito:sidebar branch 3 times, most recently Mar 6, 2016

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.css Outdated
.amp-sidebar-closed amp-sidebar,
.amp-sidebar-open amp-sidebar {
transition: transform 0.5s ease;
will-change: transform

This comment has been minimized.

@dvoytenko

dvoytenko Mar 8, 2016

Collaborator

semicolon

This comment has been minimized.

@erwinmombay

erwinmombay Mar 8, 2016

Member

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).

This comment has been minimized.

@erwinmombay

erwinmombay Mar 8, 2016

Member

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.

This comment has been minimized.

@camelburrito

camelburrito Mar 29, 2016

Collaborator

Dropping will-change for now - will consider adding it if the performance is not very good

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.css Outdated
transform: translateX(0vw) !important;
}

.amp-sidebar-closed amp-sidebar,

This comment has been minimized.

@dvoytenko

dvoytenko Mar 8, 2016

Collaborator

Please explain why classes amp-sidebar-open/close are public. I.e. what's the expected extensibility? Also, why do we set them on the parent and not the menu itself?

This comment has been minimized.

@camelburrito

camelburrito Mar 25, 2016

Collaborator

Sidebar is a page wide extension - unlike others - this might have to interact with other component on the page - I see it more like communicating the current state of the page to rest of the components.

This comment has been minimized.

@dvoytenko

dvoytenko Mar 28, 2016

Collaborator

So, what happens if the user will set amp-sidebar-open on the BODY in the initial state?

This comment has been minimized.

@camelburrito

camelburrito Mar 29, 2016

Collaborator

it will have the sidebar in the open state.


amp-sidebar {
position: fixed !important;
top: 0 !important;

This comment has been minimized.

@dvoytenko

dvoytenko Mar 8, 2016

Collaborator

This will be an issue - we have to be flexible with top positioning to accommodate page's padding.

This comment has been minimized.

@dvoytenko

dvoytenko Mar 8, 2016

Collaborator

Also, some people like to align it under their header.

This comment has been minimized.

@camelburrito

camelburrito Mar 9, 2016

Collaborator

They could always have a margin-top

This comment has been minimized.

@dvoytenko

dvoytenko Mar 10, 2016

Collaborator

It's possible. Do you think it'll be clear enough? This will also conflict slightly with your height: 100vh;. I think the only way to work it around would be by using height: calc(100vh - {top offset value})

This comment has been minimized.

@camelburrito

camelburrito Mar 25, 2016

Collaborator

I have added an offset div to be inserted as the first element when there is a viewport.paddingTop- that should take care of this

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.css Outdated
}

amp-sidebar[direction="left"] {
left: 0 !important;

This comment has been minimized.

@dvoytenko

dvoytenko Mar 8, 2016

Collaborator

I did see some cases where people wanted to put the menu in the middle of the page. What do you think about this?

This comment has been minimized.

@camelburrito

camelburrito Mar 9, 2016

Collaborator

:) since this is a sidebar - it is understandable that it would be on the sides. again this could be achieved by margin tricks as we are not being strict on that

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
this.hasMask_ = false;

if (this.direction_ != 'left' && this.direction_ != 'right') {
this.element.setAttribute('direction', 'left');

This comment has been minimized.

@dvoytenko

dvoytenko Mar 8, 2016

Collaborator

I'd expect this.direction_ to be assigned to default here as well.

This comment has been minimized.

@camelburrito

camelburrito Mar 25, 2016

Collaborator

Done

@camelburrito camelburrito force-pushed the camelburrito:sidebar branch Mar 8, 2016

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
/** @private @const {boolean} */
this.hasMask_ = false;

if (this.direction_ != 'left' && this.direction_ != 'right') {

This comment has been minimized.

@dvoytenko

dvoytenko Mar 8, 2016

Collaborator

Are we supporting dir="rtl" here as well? It could be a source of the default at the very least.

This comment has been minimized.

@camelburrito

camelburrito Mar 25, 2016

Collaborator

Yes - changed to use dir and inherit dir from the body

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
}

/** @override */
buildCallback() {

This comment has been minimized.

@dvoytenko

dvoytenko Mar 8, 2016

Collaborator

You need to also override isReadyToBuild and return false from it to avoid risk condition when building buildCallback.

This comment has been minimized.

@camelburrito

camelburrito Mar 25, 2016

Collaborator

Done

This comment has been minimized.

@mkhatib

mkhatib Mar 29, 2016

Contributor

Just fyi for me, does this basically defer building the element until onDocumentReady and forcing building all?

Also I am unclear how this race condition would happen.

* Reveals the sidebar.
* @private
*/
open_() {

This comment has been minimized.

@dvoytenko

dvoytenko Mar 8, 2016

Collaborator

Please also support activate method and just call this method from it.

This comment has been minimized.

@camelburrito

camelburrito Mar 25, 2016

Collaborator

Done

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
this.createMask_();
}

this.documentElement_.addEventListener('keydown', event => {

This comment has been minimized.

@dvoytenko

dvoytenko Mar 8, 2016

Collaborator

Also need to support history back button, like in lightbox.

This comment has been minimized.

@camelburrito

camelburrito Mar 9, 2016

Collaborator

Ack! will create bugs and followup? (let me know if you think this needs to be in the first PR)

This comment has been minimized.

@dvoytenko

dvoytenko Mar 10, 2016

Collaborator

No. Bug+TODO are fine.

This comment has been minimized.

@camelburrito

camelburrito Mar 28, 2016

Collaborator

Done

background-color: #efefef;
min-width: 30vw !important;
overflow-x: hidden !important;
overflow-y: auto !important;

This comment has been minimized.

@dvoytenko

dvoytenko Mar 8, 2016

Collaborator

Please create three bugs, assign to yourself and proceed at the earliest convenience (they will be launch blocking on this component):

  1. Possible integration with FixedLayer needed. This can be tested on iOS by scrolling and seeing if the position:fixed layout misbehaves. Also, other elements processed via FixedLayer can conflict with your logic, e.g. by being displayed above this menu.
  2. Check double-scrolling issue. Here, the test is on both iOS and Android. When scrolling the menu content, the main content should not scroll and vice-versa.
  3. Check the scrolling restoration issues on iOS and Android. E.g. open menu, scroll it and close. The main content should be unaffected by this.

This comment has been minimized.

@camelburrito

camelburrito Mar 25, 2016

Collaborator

Tested and Fixed - thanks for the pointers

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
return;
}
const mask = document.createElement('div');
mask.setAttribute('class', '-amp-sidebar-mask');

This comment has been minimized.

@dvoytenko

dvoytenko Mar 8, 2016

Collaborator

mask.classList.add(...);

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
return;
}
const mask = this.document_.createElement('div');
mask.setAttribute('class', '-amp-sidebar-mask');

This comment has been minimized.

@dvoytenko

dvoytenko Mar 28, 2016

Collaborator

Let's still use classList.add() here.

This comment has been minimized.

@camelburrito

camelburrito Mar 28, 2016

Collaborator

Done

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
const mask = this.document_.createElement('div');
mask.setAttribute('class', '-amp-sidebar-mask');
mask.addEventListener('click', () => {
this.toggle_();

This comment has been minimized.

@dvoytenko

dvoytenko Mar 28, 2016

Collaborator

I think this should be hide_ or such to avoid problems with double-click here. It's too easy to tap twice and too illogical to hide and show the menu :)

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
});
this.element.parentNode.appendChild(mask);
mask.addEventListener('touchmove', e => {
e.preventDefault();

This comment has been minimized.

@dvoytenko

dvoytenko Mar 28, 2016

Collaborator

I still don't think this is the right thing to do. To test, please add a few simple links in the menu and see if the page navigates to the target URLs. Please also demo it for me offline.

This comment has been minimized.

@camelburrito

camelburrito Mar 29, 2016

Collaborator

This is only on the mask - and yes clicking on links does work - i do have a demo on the manual test url -

https://ampskrish.herokuapp.com/test/manual/amp-sidebar.amp.html
(you will need to toggle expt) - i can demo it tomorrow when you are here.

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
this.isIosSafari_ = platform.isIos() && platform.isSafari();

if (this.direction_ != 'left' && this.direction_ != 'right') {
const pageDir = this.document_.body.getAttribute('dir') || 'ltr';

This comment has been minimized.

@dvoytenko

dvoytenko Mar 28, 2016

Collaborator

Most often I see dir attribute on <html> tag, not body. Let's at least check both places.

This comment has been minimized.

@camelburrito

camelburrito Mar 28, 2016

Collaborator

Done

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
this.fixIosElasticScrollLeak_();
}

this.adjustPadding_();

This comment has been minimized.

@dvoytenko

dvoytenko Mar 28, 2016

Collaborator

The UI styling adjustment sounds like something that should be done on open, not on build. In big part because media queries could change a lot of things.

'width': '100%',
});
const firstChild = this.element.firstChild;
this.element.insertBefore(div, firstChild);

This comment has been minimized.

@dvoytenko

dvoytenko Mar 28, 2016

Collaborator

This is hard to guarantee to work - this will skew many possible design situations. The best way is still to adjust top position. If for some reason this is not good - we can explore updating border-top. But we need to have relative position's reference point to be in the right place.

This comment has been minimized.

@camelburrito

camelburrito Mar 28, 2016

Collaborator

I already tried adding padding - and it does not seem to work for the bottom padding on IOS and adding padding/border will need vsync to compute existing style.

Adding a dummy child always (seems to) work. What do you think will not work here. I'm probably not seeing the obvious :)

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
'height': IOS_SAFARI_BOTTOMBAR_HEIGHT,
'width': '100%',
});
this.element.appendChild(div);

This comment has been minimized.

@dvoytenko

dvoytenko Mar 28, 2016

Collaborator

ditto

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
this.mutateElement(() => {
this.getViewport().addToFixedLayer(this.element);
this.createMask_();
this.viewport_.disableTouchZoom();

This comment has been minimized.

@dvoytenko

dvoytenko Mar 28, 2016

Collaborator

move above the mutateElement - we would want to reset zooming before UI mutations start.

This comment has been minimized.

@camelburrito

camelburrito Mar 28, 2016

Collaborator

Done

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
*/
open_() {
this.mutateElement(() => {
this.getViewport().addToFixedLayer(this.element);

This comment has been minimized.

@dvoytenko

dvoytenko Mar 28, 2016

Collaborator

You already have a this.viewport_ var above, no?

This comment has been minimized.

@camelburrito

camelburrito Mar 28, 2016

Collaborator

Done

* @private
*/
checkWhitelist_() {
const elements = this.element.getElementsByTagName('*');

This comment has been minimized.

@dvoytenko

dvoytenko Mar 28, 2016

Collaborator

What exactly don't we want in the menu again? I still don't understand the whitelist, but it's rather expensive.

This comment has been minimized.

@camelburrito

camelburrito Mar 29, 2016

Collaborator

ads - iframes , instagram, FB , videos , Youtube etc ... its a long list.

I would assume that there will be a limited no of elements on the sidebar (considering the space available). We could possibly remove this if/when this gets implemented in validation.

@camelburrito camelburrito force-pushed the camelburrito:sidebar branch Mar 28, 2016

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
*/
fixIosElasticScrollLeak_() {
this.element.addEventListener('scroll', e => {
this.mutateElement(() => {

This comment has been minimized.

@dvoytenko

dvoytenko Mar 28, 2016

Collaborator

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.

This comment has been minimized.

@camelburrito

camelburrito Mar 28, 2016

Collaborator

It seems to work well in mutateElement and the direct approach. removing the mutate now.

@dvoytenko

View changes

extensions/amp-sidebar/0.1/amp-sidebar.js Outdated
/**
* @private
*/
compensateSafariNavBarHeight_() {

This comment has been minimized.

@dvoytenko

dvoytenko Mar 28, 2016

Collaborator

remove?

This comment has been minimized.

@camelburrito

camelburrito Mar 28, 2016

Collaborator

Done Lol- that should have been pretty obvious.

@dvoytenko

View changes

tools/experiments/experiments.js Outdated
@@ -50,6 +50,22 @@ const EXPERIMENTS = [
spec: 'https://github.com/ampproject/amphtml/blob/master/' +
'README.md#amp-dev-channel',
},

// Dynamic CSS Classes

This comment has been minimized.

@dvoytenko

dvoytenko Mar 28, 2016

Collaborator

This has been removed. Please remove as well.

This comment has been minimized.

@camelburrito

camelburrito Mar 28, 2016

Collaborator

Done

@camelburrito camelburrito force-pushed the camelburrito:sidebar branch 8 times, most recently Mar 28, 2016

camelburrito added some commits Feb 25, 2016

amp-sidebar
- 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
Move state classes to amp-sidebar and make them attributes - change l…
…ayout type to no-display and add aria support.

@camelburrito camelburrito force-pushed the camelburrito:sidebar branch to e4647c7 Apr 1, 2016

@camelburrito

This comment has been minimized.

Copy link
Collaborator

camelburrito commented Apr 13, 2016

split and submitted as smaller PRs

#2788 , #2792, #2795 , #2811, #2812, #2813, #2823

@camelburrito camelburrito deleted the camelburrito:sidebar branch Apr 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment