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

Changed all component child lists to arrays instead of objects #2477

Closed
wants to merge 1 commit into from

Conversation

heff
Copy link
Member

@heff heff commented Aug 13, 2015

Udpated the docs to match. This is a necessary change because object key order is not consistent in Chrome and children have a specific order. Somehow this hasn't actually affect us yet. But also there's not an easy way to insert a child into a specific spot in the order if it's an object.

The biggest issue here is that player children has been changed to an array. This would break anywhere that previously used

// any control bar or control bar child options under the children key. Same for other player children.
playerOptions = { children: { controlBar: { ... } } };
// Add a child to the children object
playerOptions = { children: { newChild: {} } };

Child options will no longer merge when they're under the children key, because children is now an array by default that will simply be overwritten by the new children object when options are merged.

You instead do:

playerOptions = { controlBar: { ... } };
// I'm not sure if adding children through options is real use case, but it wouldn't work with the array
// you can still use the addChild API, which I think is the primary one
player.addChild('newChild');
// or the prototype options
Player.prototype.options_.children.push('newChild'); // though this API has changed anyway

I know this is important but also don't have a good grasp of the significance, in case anyone else has any additional input. If we think it's really significant we can look into a patch or reconsider.

@pam
Copy link

pam commented Aug 13, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: e499f1a024bd31d1f3f77689b1190615e01a83d3
Build details: https://travis-ci.org/pam/video.js/builds/75504974

(Please note that this is a fully automated comment.)

@heff
Copy link
Member Author

heff commented Sep 1, 2015

@dmlap @gkatsev, this is one you guys should probably review.

@dmlap
Copy link
Member

dmlap commented Sep 1, 2015

Is using children object just deprecated or is the intent to completely remove it? I understand the motivation for pushing towards an array but it's a pretty pervasive pattern.

@heff
Copy link
Member Author

heff commented Sep 1, 2015

I don't feel a strong need to remove it completely, but it will probably stop being used either way if the internal components aren't using it. There won't be any value left in using the object approach.

@heff
Copy link
Member Author

heff commented Sep 1, 2015

@dmlap see #2534 for emphasis on why this is needed.

@heff heff added this to the v5.0.0 milestone Sep 24, 2015
@heff
Copy link
Member Author

heff commented Sep 24, 2015

This one got lost for a bit but needs to go in before 5.0. Can I get another set of eyes?

@heff
Copy link
Member Author

heff commented Sep 27, 2015

Since no one else has anything to say on this one I'm going to pull it in on Monday.

@mmcc
Copy link
Member

mmcc commented Sep 28, 2015

lgtm

@heff heff closed this in 1d79c4a Sep 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants