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

Array traversing for children opts - closes #1070 #1093

Merged
merged 1 commit into from May 6, 2014

Conversation

Projects
None yet
3 participants
@Akkuma
Contributor

Akkuma commented Mar 18, 2014

This should allow two additional ways to specify children for turning on specific components and ordering them:

children: [
  'comp1',
  'comp2',
  'comp3'
]
children: [
  { comp1: {} },
  { comp2: {} },
  { comp3: {} }
]

There is no an additional vjs.each, which can handle either objects or arrays

@mmcc

This comment has been minimized.

Show comment
Hide comment
@mmcc

mmcc Mar 19, 2014

Member

Cool! I'm going to wait until @heff is back in town to dive in, but at first glance there seems to be some weird stuff going on with indentation. Mind double checking on that and making sure things match the rest of the project in terms of style convention?

Member

mmcc commented Mar 19, 2014

Cool! I'm going to wait until @heff is back in town to dive in, but at first glance there seems to be some weird stuff going on with indentation. Mind double checking on that and making sure things match the rest of the project in terms of style convention?

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Mar 24, 2014

Member

Thanks for this. I get the need for this, since chrome will reorder object keys and potentially change the order of children, which is bad. This should definitely be fixed. The changes in this pull request are a little hard to track because it looks like your editor changed a lot of the spacing around things. Can you remove the spacing changes that don't specifically apply to this?

Member

heff commented Mar 24, 2014

Thanks for this. I get the need for this, since chrome will reorder object keys and potentially change the order of children, which is bad. This should definitely be fixed. The changes in this pull request are a little hard to track because it looks like your editor changed a lot of the spacing around things. Can you remove the spacing changes that don't specifically apply to this?

@Akkuma

This comment has been minimized.

Show comment
Hide comment
@Akkuma

Akkuma Apr 7, 2014

Contributor

@heff I added in a commit to fix the spacing issues. The actual code I checked in that is new is very little:

Contributor

Akkuma commented Apr 7, 2014

@heff I added in a commit to fix the spacing issues. The actual code I checked in that is new is very little:

@Akkuma

This comment has been minimized.

Show comment
Hide comment
@Akkuma

Akkuma Apr 14, 2014

Contributor

@heff Here is a link with all the whitespace removed: https://github.com/videojs/video.js/pull/1093/files?w=1

Github has the feature baked in of adding ?w=1 onto the end to ignore all whitespace changes.

Contributor

Akkuma commented Apr 14, 2014

@heff Here is a link with all the whitespace removed: https://github.com/videojs/video.js/pull/1093/files?w=1

Github has the feature baked in of adding ?w=1 onto the end to ignore all whitespace changes.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 25, 2014

Member

Going to think in the open a bit here...

The order of components is important, and I'm surprised we haven't run into issues with Chrome and the default children option for the player. Chrome doesn't maintain order, so we could have ended up with components hiding other components.

So an array would be better to use, however there's currently a feature of using an object here that allows you to disable just one of the child components instead of rebuilding the whole children list. e.g. the following would block the bigPlayButton from being added

videojs(id, {
  children: {
    bigPlayButton: false
  }
});

If we used an array for the children option, you couldn't do that. I'm not really sure that it's a big deal to lose that, but it's possible people are using it. Either way, we don't have to change the default children object to satisfy this specific issue, we just have to allow passing in an array.

As far as the array items, when they include options, I think it'd be better if we include an component option setting to provide the class type of the child. Possibly just name. e.g.

children: [
  { 
    name: comp1,
    otherOption: true 
  }
]

As opposed to the originally suggested:

children: [
  {
    comp1: {
      otherOption: true 
    } 
  }
]

As far as adding .each(), I think that's making this PR do too much. I've avoided adding a 'smart' each() method so far because it seemed unnecessary since we're not building/exposing a utility lib, and I don't know of any other place where we're not sure if we're dealing with an array or object.
We've started discussing what it would look like to pull in lodash, and I could see that as a good time to add each() if we're going to.

How do you feel about those updates?

Member

heff commented Apr 25, 2014

Going to think in the open a bit here...

The order of components is important, and I'm surprised we haven't run into issues with Chrome and the default children option for the player. Chrome doesn't maintain order, so we could have ended up with components hiding other components.

So an array would be better to use, however there's currently a feature of using an object here that allows you to disable just one of the child components instead of rebuilding the whole children list. e.g. the following would block the bigPlayButton from being added

videojs(id, {
  children: {
    bigPlayButton: false
  }
});

If we used an array for the children option, you couldn't do that. I'm not really sure that it's a big deal to lose that, but it's possible people are using it. Either way, we don't have to change the default children object to satisfy this specific issue, we just have to allow passing in an array.

As far as the array items, when they include options, I think it'd be better if we include an component option setting to provide the class type of the child. Possibly just name. e.g.

children: [
  { 
    name: comp1,
    otherOption: true 
  }
]

As opposed to the originally suggested:

children: [
  {
    comp1: {
      otherOption: true 
    } 
  }
]

As far as adding .each(), I think that's making this PR do too much. I've avoided adding a 'smart' each() method so far because it seemed unnecessary since we're not building/exposing a utility lib, and I don't know of any other place where we're not sure if we're dealing with an array or object.
We've started discussing what it would look like to pull in lodash, and I could see that as a good time to add each() if we're going to.

How do you feel about those updates?

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 26, 2014

Member

Just for reference, videojs/videojs-youtube#74 is an example of someone using the comp: false method I mentioned.

Member

heff commented Apr 26, 2014

Just for reference, videojs/videojs-youtube#74 is an example of someone using the comp: false method I mentioned.

heff added a commit to heff/video.js that referenced this pull request May 6, 2014

@Akkuma

This comment has been minimized.

Show comment
Hide comment
@Akkuma

Akkuma May 6, 2014

Contributor

I originally didn't add in the alternate syntax for the array as I was trying to maintain the same style that was used in videojs, but I definitely like this a lot better, since it removes the unnecessary overhead.

Contributor

Akkuma commented May 6, 2014

I originally didn't add in the alternate syntax for the array as I was trying to maintain the same style that was used in videojs, but I definitely like this a lot better, since it removes the unnecessary overhead.

@heff heff merged commit e7617bf into videojs:master May 6, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 6, 2014

Member

Awesome, thanks!

Member

heff commented May 6, 2014

Awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment