Add new helpers for adding children to components

[gkatsev]

To add a new child component we currently have addChild with the following method signature:

// child can be either a string like 'volumePanel' or an instance of an object,
// in which change options shouldn't be provided
addChild(child [, options [, index]]);

This method is quite similar to appendChild in the DOM, except that we also have a position where the component can be added as opposed to just at the end. Often, we have a particular control, and we want to insert a new child next to it. To do so, we'd need to get access to the control, loop through the children until we find the control and then we can actually call addChild. This seems like a lot of extra work that Video.js users have to do each time they want to add a child component to a parent at a specific position. Looking at the DOM for inspiration, I propose the addition of 3 new methods: replaceChild, insertChildBefore, insertChildAfter. Child is in the name to be clear what these methods do given our mishmash of APIs on Components. insertChildAfter is also incredibly useful, especially given that we have no way to easily know what the next sibling component is.

Method signatures:

// oldChild must be provided
replaceChild(newChild [, newChildOptions [, oldChild]]);

// if a reference child isn't provided, the child should be appended
insertChildBefore(child [, childOptions [, referenceChild]]);

// if a reference child isn't provided, the child should be appended?
insertChildAfter(child [, options [, referenceChild]]);

Examples:

// replaceChild
player.controlBar.replaceChild('volumePanel', {inline: false}, player.controlBar.volumePanel);

// insertChildBefore
player.controlBar.insertChildBefore('settingsMenuButton', {}, player.controlBar.fullscreenToggle);

// insertChildAfter
player.controlBar.insertChildAfter('nextPlaylistItemButton', {}, player.controlBar.playToggle);

Previously, to do the above you would've had to do something like this:

// if you aren't assuming that findIndex is available, the code is even worse
var volumePanelIndex = player.controlBar.children().findIndex((child) => child === player.controlBar.volumePanel);
player.controlBar.removeChild(player.controlBar.volumePanel);
player.controlBar.volumePanel = player.controlBar.addChild('volumePanel', {inline: false}, 1);

[gkatsev]

What should the default behavior be for insertChildAfter if referenceChild isn't provided?

[gkatsev]

Also, names are totally up for renaming, too.

[AwokeKnowing]

I suggest that

insertChildBefore('VolumePanel', {inline:false}) //no reference. same as 'prepend' insertChildAfter('VolumePanel', {inline:false}) //no reference. same as 'append' replaceChild('VolumePanel', {inline:false}) //no reference. check if VolumePanel exists. if so, replace, otherwise append

In practice it could be common to end up with a null ref, and so the defaults would be handled predictably. in addition they'd even be fairly convenient if you were practically expecting the reference to not be defined. and for replace it doubles as convenient syntax.

On the other hand, I feel that putting the reference first is actually much better, as in

insert after 'playtoggle': volume panel insert before 'caption menu': transcript button replace 'volume pane': a new volume panel

The way English works is Verb Preposition Object. So if you read 'Insert Before Volume Panel' you expect something to be inserted before the volume panel. IMHO

[gkatsev]

We could potentially look to see if a matching element exists for replaceChild assuming that a string was used rather than a component. I guess I should update my example. Because you are able to instantiate a Component manually and then call addChild with that.

[gkatsev]

I was basing my initial suggestion for default behavior for insertChildBefore based on the default behavior of insertBefore in the DOM, though, that doesn't mean it's necessarily the best answer.

Making optional arguments be the first item in a list of arguments in js is quite tricky. Plus, given how closely this mirrors the DOM API, I'd be wary to make it very different.

[AwokeKnowing]

videojs.getComponent('Component').prototype.replaceChild = function(child, options, newChild) {
    let pos = -1
    this.children_.map((childObj, childPos) => 
        pos = (childObj.name_ === child || childObj === child)?childPos:pos)
    
    if(pos === -1){
        this.addChild(child, options)
    }else{
        this.removeChild(child)
        this.addChild(newChild || child, options, pos);
    }
}

here's a concrete implementation of replaceChild for reference. I think when you're putting something 'before' or 'after' something it's common that you expect that 'thing' that is the basis of the 'before/after' to be there, most of the time. So in my view, the primary parameter is infact the object to which new objects are relative. So if I'm putting something after foo, seems foo would be the first argument putSomethingAfter(foo). foo = new Foo() someParent.addChild(foo) someParent.insertChildAfter(foo, 'VolumePanel')

it does get tricky because sometimes we're talking about optional references, and other times the reference is the main thing we're 'working on' eg

ExecuteSQL("SELECT 1", context) is common because context might even be set some other way, and omitting it is convenient.

but notice that the 'english' works out very nicely Execute Select 1. same for eg

replace('old', 'new', someString)  //or someString.replace('old', 'new')

the key 'thought' is 'replace old with new', the rest is context. So I'm arguing that 'insert after X' means 'insert after (X)' (though here it's not verb object but preposition object)

(that all said, in programmer world, optional parameters may be needed, in which case it can be better to just go ahead and reference the 'context' first, then then new thing with it's new, possibly optional, options, even if it means breaking the 'verb' rule. So I have no strong opinion)

[gkatsev]

Thinking about this some more, I'd like to investigate how hard it would be to implement the methods akin to the ChildNode methods https://developer.mozilla.org/en-US/docs/Web/API/ChildNode.

[gkatsev]

Definitely some more thought required here.

[AwokeKnowing]

That seems the best approach to me.

[mister-ben]

Another possibility is to allow the third argument of addChild to be an object of options to allow flexibility. component.addChild(newComponent, {}, {after: referenceNode}) or component.addChild(newComponent, {}, {position: 'after', ref: referenceNode})

Similar to how positional arguments got annoying with addEventListener so it uses an options object for new features.

[AwokeKnowing]

so @mister-ben, i suppose could be both, where the individual children have .after() .before() .replaceWith and the parent just has some extensions to existing methods. maybe addChild(newChild, {}, {before|after|replace: 'existingChild'})

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.