-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
🐛 [Story attachment] Opening outlink error for closeButton being null #37833
Conversation
Hey @gmajoulet, @newmuis! These files were changed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test?
For sure, I think in general the feature is undertested but any unit test that checks this functionality should have picked it up. I'll add a generic test for the outlink component that would have caught this error (and potentially many other ones like it). |
extensions/amp-story-page-attachment/0.1/test/test-amp-story-page-attachment.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding tests!
Thank you for fixing this & writing a test for it ❤️ |
Fixes ampproject/error-reporting#147
Outlinks when opened don't have a
closeButton
that they can change thetabindex
so it was throwing an error. By checking if there's such button we can prevent the error and run the last lines of theopen()
method properly.