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.

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

Fix amp-image won't render after resize in amp-fx-flying-carpet with media attribute #16809

Merged
merged 4 commits into from
Jul 17, 2018

Conversation

cathyxz
Copy link
Contributor

@cathyxz cathyxz commented Jul 17, 2018

Closes #11621

@codecov-io
Copy link

Codecov Report

Merging #16809 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16809      +/-   ##
==========================================
- Coverage   78.09%   78.02%   -0.08%     
==========================================
  Files         558      558              
  Lines       40437    40438       +1     
==========================================
- Hits        31578    31550      -28     
- Misses       8859     8888      +29
Flag Coverage Δ
#integration_tests 34.85% <ø> (-0.29%) ⬇️
#unit_tests 77.05% <100%> (-0.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ea9bde...4af7773. Read the comment docs.

@cathyxz cathyxz requested review from jridgewell and alanorozco July 17, 2018 20:39
Copy link
Member

@alanorozco alanorozco left a comment

Choose a reason for hiding this comment

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

ezpz!

@cathyxz cathyxz merged commit a08806c into ampproject:master Jul 17, 2018
@cathyxz cathyxz deleted the bugfix/carpet branch July 17, 2018 20:55
setStyle(this.container_, 'width', width, 'px');
});
this.scheduleLayout(dev().assertElement(this.container_));
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be reverted. It can only schedule layout for the container if it itself has been laid out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can onMeasureChanged get called before layout?

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be called after build.

Copy link
Contributor Author

@cathyxz cathyxz Jul 17, 2018

Choose a reason for hiding this comment

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

Revert PR: #16816

cathyxz added a commit that referenced this pull request Jul 17, 2018
gopanisandip pushed a commit to gopanisandip/amphtml that referenced this pull request Jul 27, 2018
…media attribute (ampproject#16809)

* Add flying carpet media test

* Schedule layout of flying carpet container on onMeasureChanged

* Add unit test

* Assert element tand force non null
@mrjoro mrjoro mentioned this pull request Aug 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants