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

🐛 amp-story: Use listen and remove listeners on unlayoutcallback. #17004

Conversation

juanlizarazo
Copy link
Member

@juanlizarazo juanlizarazo commented Jul 23, 2018

This PR does the following:

  • Changes how event listeners are attached: refactored addEventListener to use the listen event helper instead.
  • Calls unlisteners from onlayoutCallback.

Refactor for #13159

c.c. @mrjoro

@codecov-io
Copy link

Codecov Report

Merging #17004 into master will increase coverage by 0.01%.
The diff coverage is 8.69%.

@@            Coverage Diff             @@
##           master   #17004      +/-   ##
==========================================
+ Coverage   78.01%   78.02%   +0.01%     
==========================================
  Files         562      562              
  Lines       41081    41077       -4     
==========================================
+ Hits        32048    32052       +4     
+ Misses       9033     9025       -8
Flag Coverage Δ
#integration_tests 36.16% <ø> (ø) ⬆️
#unit_tests 77.08% <8.69%> (+0.01%) ⬆️

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 ca0b8da...01bed40. Read the comment docs.

@juanlizarazo juanlizarazo changed the title amp-story: Use listen and remove listeners on unlayoutcallback. :recycle amp-story: Use listen and remove listeners on unlayoutcallback. Jul 23, 2018
@juanlizarazo juanlizarazo changed the title :recycle amp-story: Use listen and remove listeners on unlayoutcallback. ♻️ amp-story: Use listen and remove listeners on unlayoutcallback. Jul 23, 2018
@juanlizarazo juanlizarazo changed the title ♻️ amp-story: Use listen and remove listeners on unlayoutcallback. 🐛 amp-story: Use listen and remove listeners on unlayoutcallback. Jul 23, 2018
@mrjoro mrjoro requested a review from newmuis July 23, 2018 14:55
Copy link
Contributor

@newmuis newmuis left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in review; this PR slipped through the cracks.

This seems like it would break the behavior of going to a different tab and coming back. Do these somehow get re-registered when returning to the tab?

@juanlizarazo
Copy link
Member Author

Closing per findings in original issue #17004.

@juanlizarazo juanlizarazo deleted the 13159-use-listen-in-amp-story branch October 17, 2018 04:26
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.

4 participants