Skip to content
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.

Sign up for GitHub

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

Jump to bottom

amp-lightbox relayout children when transitionend #10811

Merged
merged 8 commits into from Aug 14, 2017
Merged

amp-lightbox relayout children when transitionend #10811

merged 8 commits into from Aug 14, 2017

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented Aug 4, 2017

Schedule relayout to amp-iframe element, if placed inside <amp-lightbox> <amp-sidebar>.
Rewrite amp-iframe test.
fix #10657

@zhouyx zhouyx force-pushed the reschedule-layout-lightbox branch from 58e3945 to 85ed604 Compare August 7, 2017 22:21
dvoytenko
dvoytenko suggested changes Aug 7, 2017
View reviewed changes
extensions/amp-lightbox/0.1/amp-lightbox.js Outdated
*/
reschedule_(event) {
if (event.type == 'transitionend') {
// Ignore the first transitionend event.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amp-lightbox by default apply transition: 'opacity 0.1s ease-in' to its element. I am trying to ignore this specific transitionend event here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(a) can we be more specific, i.e. by looking into event.target or specific properties?
(b) given debounce don't we run a risk of missing the followup events?
(c) is this a bit deal if we proceed an extra time? I think runtime does additional debouncing naturally, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a) I found it hard to use event property here. 'target' is not a good property to check since transition apply to lightbox element/ iframe element/iframe child element can all result in iframe size change.
b) c). Yes! I agree an extra scheduleLayout doesn't hurt. changed.

@zhouyx zhouyx force-pushed the reschedule-layout-lightbox branch from 85ed604 to 381ade2 Compare August 7, 2017 22:49
@zhouyx zhouyx force-pushed the reschedule-layout-lightbox branch from bb285ca to 3d3ff9c Compare August 8, 2017 18:47
jridgewell
jridgewell requested changes Aug 9, 2017
View reviewed changes
extensions/amp-iframe/0.1/amp-iframe.js Outdated
// Iframe is not tracking iframe if open with user interaction
if (this.isInContainer_ === undefined) {
let ele = this.element;
do {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just Element#closest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

extensions/amp-lightbox/0.1/amp-lightbox.js
@@ -152,6 +160,8 @@ class AmpLightbox extends AMP.BaseElement {
}
// TODO: instead of laying out children all at once, layout children based
// on visibility.
this.element.addEventListener('transitionend', this.boundReschedule_);
this.element.addEventListener('animationend', this.boundReschedule_);
this.scheduleLayout(container);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these calls necessary now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. would still be useful for elements with relayoutNeeded = true. amp-img for example.

@zhouyx
Copy link
Contributor Author

zhouyx commented Aug 10, 2017

PTAL

@zhouyx
Copy link
Contributor Author

zhouyx commented Aug 11, 2017

Friendly ping : )

dvoytenko
dvoytenko suggested changes Aug 11, 2017
View reviewed changes
extensions/amp-iframe/0.1/amp-iframe.js Outdated
return true;
// Iframe is not tracking iframe if open with user interaction
if (this.isInContainer_ === undefined) {
this.isInContainer_ = !!closest(this.element, element => {
Copy link
Contributor

@dvoytenko dvoytenko Aug 11, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be faster to just do a DOM query, as in closestBySelector(this.element, 'amp-lightbox|amp-sidebar'). And even better is to define a shared class and do:

closestBySelector(this.element, '.i-amphtml-overlay');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied 'i-amphtml-overlay'. I have concern on how people know about these classes when creating new components though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we need to think about it. I'd recommend "copy paste" lightbox at this point :) But it's invariant here since devs still need to remember to whitelist name here otherwise.

dvoytenko
dvoytenko approved these changes Aug 11, 2017
View reviewed changes
Copy link
Contributor

@dvoytenko dvoytenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, but otherwise LGMT on my side.

extensions/amp-iframe/0.1/amp-iframe.js Outdated
@@ -44,6 +44,12 @@ const ATTRIBUTES_TO_PROPAGATE = [
'scrolling',
];

/** @const {!Array<string>} */
const CONTAINER_LIST = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer needed?

@zhouyx
Copy link
Contributor Author

zhouyx commented Aug 14, 2017

@jridgewell PTAL

@zhouyx zhouyx merged commit 60a4045 into ampproject:master Aug 14, 2017
@zhouyx zhouyx deleted the reschedule-layout-lightbox branch August 14, 2017 18:08
@dreamofabear
Copy link

Looks like this PR is causing a type error:

extensions/amp-lightbox/0.1/amp-lightbox.js:88: ERROR - variable AmpLightbox$$module$extensions$amp_lightbox$0_1$amp_lightbox.prototype.buildCallback redefined, original definition at extensions/amp-lightbox/0.1/amp-lightbox.js:78
  buildCallback() {
  ^

1 error(s), 0 warning(s)

https://travis-ci.org/ampproject/amphtml/jobs/264459032

@dreamofabear dreamofabear mentioned this pull request Aug 14, 2017
Merged
@zhouyx
Copy link
Contributor Author

zhouyx commented Aug 14, 2017 •
edited

#10910
Oh! Fail to sync before merge....

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

@jridgewell jridgewell jridgewell approved these changes

@dvoytenko dvoytenko dvoytenko approved these changes

@aghassemi aghassemi Awaiting requested review from aghassemi

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
5 participants
@zhouyx @dreamofabear @jridgewell @dvoytenko @googlebot