feat: Player can be replaced with original el after dispose() #7722
Conversation
Codecov Report
@@ Coverage Diff @@
## main #7722 +/- ##
==========================================
- Coverage 80.91% 80.91% -0.01%
==========================================
Files 116 116
Lines 7457 7461 +4
Branches 1806 1810 +4
==========================================
+ Hits 6034 6037 +3
- Misses 1423 1424 +1
Continue to review full report at Codecov.
|
misteroneill
added
the
minor
There was a problem hiding this comment.
Confirmed with:
var d= document.createElement('div')
d.innerHTML = '<span style="color: red; font-size: 30px;">RESTORED</span>'
player.options_.restoreEl = d
player.dispose()If you're able to add the other tests, that would be awesome, but I think it's probably ok as is.
src/js/video.js
Outdated
| @@ -162,6 +162,11 @@ function videojs(id, options, ready) { | |||
|
|
|||
| options = options || {}; | |||
|
|
|||
| // Store a copy of the el before modification, if it is to be restored in destroy() | |||
| if (options.restoreEl === true) { | |||
| options.restoreEl = el.cloneNode(true); | |||
There was a problem hiding this comment.
Not a block: Clever to re-use the option to store the clone element, though, that might be a bit unexpected.
src/js/video.js
Outdated
| @@ -162,6 +162,11 @@ function videojs(id, options, ready) { | |||
|
|
|||
| options = options || {}; | |||
|
|
|||
| // Store a copy of the el before modification, if it is to be restored in destroy() | |||
| if (options.restoreEl === true) { | |||
| options.restoreEl = el.cloneNode(true); | |||
There was a problem hiding this comment.
to get this line covered by tests, we probably want to add a test to https://github.com/videojs/video.js/blob/main/test/unit/video.test.js
Ideally, we'd have a test per embed type https://videojs.com/guides/embeds/
There was a problem hiding this comment.
I'll add some more tests. And sine this can be a minor I'll rebase onto main. Before starting I was thinking this would be more impactful a change than it is.
mister-ben
force-pushed
the
feat-resset-dom
branch
from
5172487
to
5fb9c9d
Compare
| ]; | ||
|
|
||
| embeds.forEach(embed => { | ||
| const comparisonEl = document.createRange().createContextualFragment(embed.src).children[0]; |
There was a problem hiding this comment.
huh, never seen createContextualFragment before
test/unit/video.test.js
Outdated
|
|
||
| const player = videojs(embed.initSelector, {restoreEl: true}); | ||
|
|
||
| assert.ok(comparisonEl.isEqualNode(player.options_.restoreEl), `${embed.type}: restoreEl option replaced by element`); |
There was a problem hiding this comment.
non-blocking: should we check that player.el() isn't equal to comparisonEl?
Description
Adds an option to replace player with a clone of the original element on dispose, which has been asked for several times.
Specific Changes proposed
options.restoreEl.restoreEltosuper.dispose()if provided as a player optionvideojs()constructor accepts arestoreEloption, which can be any element, ortruein which case the element the player is initialised on is used.Docs will need to be added at videojs.com/guides
Requirements Checklist