Fix TypeError when calling unbindVideoElement #3043
Conversation
| monitor.bindVideoElement(null); | ||
| }); | ||
|
|
||
| it('should skip unobserve if no element is being beserved', () => { |
| const videoElementFactory = new NoOpVideoElementFactory(); | ||
| const videoElement = videoElementFactory.create(); | ||
| monitor.bindVideoElement(videoElement); | ||
| monitor.bindVideoElement(null); |
There was a problem hiding this comment.
do we need some expect statements for this test case?
There was a problem hiding this comment.
The function is expected to handle these inputs. Added expect no exception to throw
| const videoElementFactory = new NoOpVideoElementFactory(); | ||
| const videoElement = videoElementFactory.create(); | ||
| expect(() => monitor.bindVideoElement(videoElement)).to.not.throw(); | ||
| expect(() => monitor.bindVideoElement(null)).to.not.throw(); |
There was a problem hiding this comment.
I think it will good to expect this.resizeObserver.observe() and this.resizeObserver.unobserve() to havebeencalled(times) in both tests.
There was a problem hiding this comment.
Added boolean to represent if observe or unobserve is called in unit test and expect on them.
| return; | ||
| } | ||
| this.element = newElement; |
There was a problem hiding this comment.
Q: if newElement is null, is it supposed to be observed?
There was a problem hiding this comment.
If newElement it will be handled inside the if (!newElement) section above call unobserve if it was observing an element
| if (this.element) { | ||
| this.resizeObserver.unobserve(this.element); | ||
| } | ||
| this.element = newElement; |
There was a problem hiding this comment.
explicit this.element = null should be good.
There was a problem hiding this comment.
I like this change, makes it a bit easier to read
There was a problem hiding this comment.
I noticed ResizeObserver can observe multiple elements. So I updated the function to unobserve the element being observed if not null, and then observe newElement if not null. Also added a function to skip when newElement equals to the one being observed
Issue #: #3042
Description of changes:
Fix TypeError when calling unbindVideoElement
Testing:
Smoke test and verified TypeError is not thrown when calling unbindVideoElement
Can these tested using a demo application? Please provide reproducible step-by-step instructions.
video tile updatedand notetileIdof the latest tilethis.app.audioVideo.unbindVideoElement(<tileId>)Checklist:
Have you successfully run
npm run build:releaselocally?yes
Do you add, modify, or delete public API definitions? If yes, has that been reviewed and approved?
no
Do you change the wire protocol, e.g. the request method? If yes, has that been reviewed and approved?
no
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.