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

Clean up and documentation of src/js/video.js and DOM functions #2182

Merged
merged 1 commit into from
May 21, 2015

Conversation

heff
Copy link
Member

@heff heff commented May 20, 2015

  • Standardized naming of dom functions in preparation of exporting on the videojs object.
  • Cleaned up an documented a lot of the main video.js source file
  • Fixed an issue with the new mergeOptions function. It no longer makes a clone of the first object, but does make new objects for empty keys that are being merged with, to match how lodash.merge works by default.

@@ -70,7 +68,7 @@ export const insertFirst = function(child, parent){
* @type {Object}
* @private
*/
export const cache = {};
const _data = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this isn't exported, there's no need for the _.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Is that the common practice? Add an underscore only to private vars that are going to be accessible externally?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.

@gkatsev
Copy link
Member

gkatsev commented May 20, 2015

other than the _, LGTM.

Options['languages'][code] = data;
}
return Options['languages'];
return merge(globalOptions.languages, { [code]: data })[code];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tumblr_m2su7wfsrr1qhlnuu

Preparing to export utility functions on the videojs object

closes videojs#2182

Change el() to getEl() for consistency

Cleaned up DOM functions library

Clean up and document videojs object API

Fixed mergeOptions to modify the first object instead of a copy

More cleanup of the main video.js file and documentation

Fixed issues with mergeOptions

Cleaned up the addLanguage function

Removed unnecessary underscores in private module vars
@heff heff merged commit 1bfe0b4 into videojs:master May 21, 2015
@heff heff deleted the wildcards branch May 22, 2015 03:05
@heff heff mentioned this pull request May 27, 2015
@heff heff mentioned this pull request Aug 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants