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-fx-flying carpet initial alignment is broken #17223

Closed
aghassemi opened this issue Jul 31, 2018 · 5 comments
Closed

amp-fx-flying carpet initial alignment is broken #17223

aghassemi opened this issue Jul 31, 2018 · 5 comments
Assignees
@aghassemi
Labels
P0: Drop Everything

Comments

@aghassemi
Copy link
Contributor

Only reproducible with current 7/26 Canary (1532641868525)
Tested on OSX/Chrome with https://ampbyexample.com/components/amp-fx-flying-carpet/

Prod:
prod-fx-flying

Canary:
canary-fx-flying

Setting to P0 since it is a regression in Canary and breaks fx-flying-carpet.

Introduced by either #16809 or #16821

Needs to be fixed or reverted for cherry pick on #17097

/cc @jridgewell any thoughts for a quick fix?
@aghassemi aghassemi self-assigned this Jul 31, 2018
@rsimha
Copy link
Contributor

rsimha commented Jul 31, 2018

Once the P0 issue is fixed, we should add a better visual test for this component, since the ABE test page doesn't adequately test it.

See #11418

/cc @danielrozenberg

@jridgewell
Copy link
Contributor

Dropping to P1, since it's only affecting opt-in canary.

@aghassemi
Copy link
Contributor Author

@jridgewell I am using P0 for anything that's blocking release this week.

@jridgewell
Copy link
Contributor

onMeasureChanged isn't fired when the custom element is upgraded.

@aghassemi
Copy link
Contributor Author

(issue is in 1% canary that was supposed to go to prod today. I am delaying prod until this and few other ones are patched before promoting)

This was referenced Jul 31, 2018
Merged
Closed
jridgewell added a commit to jridgewell/amphtml that referenced this issue Aug 3, 2018
@jridgewell
Remove temporary fix for ampproject#17223
4600e9b
jridgewell added a commit to jridgewell/amphtml that referenced this issue Aug 3, 2018
@jridgewell
First onMeasureChanged even if unchanged after build
0a3a7a6 
This is the real fix for ampproject#17223.
Reverts ampproject#17225.
Fixes ampproject#17228.
@jridgewell jridgewell mentioned this issue Aug 3, 2018
Merged
jridgewell added a commit that referenced this issue Aug 4, 2018
@jridgewell
🐛✨First onMeasureChanged even if unchanged after build (#17294)
4f52f81 
* First onMeasureChanged even if unchanged after build

This is the real fix for #17223.
Reverts #17225.
Fixes #17228.

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

@aghassemi aghassemi

Labels
P0: Drop Everything
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jridgewell @aghassemi @rsimha