Skip to content

Expose DOM Helpers #2754

Closed
wants to merge 11 commits into from

2 participants

@misteroneill
Video.js member

Building on the ModalDialog work...

  • Adds a couple small DOM convenience methods - $ and $$ (essentially, querySelector and querySelectorAll respectively).
  • Adds a toggleElClass function.
  • Exposes a bunch of useful DOM methods on the videojs function.
  • Rewrites the className manipulation functions to use classList if it's available (which is true of all supported browsers except IE8).

Note: The methods are generally exposed without the El part (i.e. Dom.hasElClass is exposed as videojs.hasClass). The reason for this is parity with popular JS libraries (jQuery, etc) that people are familiar with.

@gkatsev gkatsev commented on the diff
src/js/tracks/text-track-list-converter.js
@@ -44,6 +44,8 @@ let trackToJson_ = function(track) {
* @function textTracksToJson
*/
let textTracksToJson = function(tech) {
+
+ // Cannot use $$ here because it is not an instance of Tech
let trackEls = tech.el().querySelectorAll('track');
@gkatsev
Video.js member
gkatsev added a note

isn't tech here a Tech and thus it should have $$?

@misteroneill
Video.js member

It wasn't when I was working on this PR (hence the comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gkatsev gkatsev and 1 other commented on an outdated diff
src/js/video.js
+ * Finds a single DOM element matching `selector` within the optional
+ * `context` of another DOM element (defaulting to `document`).
+ *
+ * @method $
+ * @param {String} selector
+ * A valid CSS selector, which will be passed to `querySelector`.
+ *
+ * @param {Element|String} [context=document]
+ * A DOM element within which to query. Can also be a selector
+ * string in which case the first matching element will be used
+ * as context. If missing (or no element matches selector), falls
+ * back to `document`.
+ *
+ * @return {Element|null}
+ */
+videojs.$ = Dom.$;
@gkatsev
Video.js member
gkatsev added a note

I'm worried that exposing $ and $$ will make people assume that we either have jquery or are jquery compatible. We also don't want people do be reliant on videojs for that kind of functionality.
I think the other functions are totally fine, though.

@misteroneill
Video.js member

That's fair and makes a ton of sense. To be clear: do you want to keep them internally and on components or ditch them entirely? I'm fine with either - they are the least useful part of this PR, I think.

@gkatsev
Video.js member
gkatsev added a note

Keeping them internal is fine, since doing .el().querySelector seems very common.
So, just remove them externally.

@misteroneill
Video.js member

And keeping them on Component.prototype, right? The only concern there is they are sort-of-exposed (e.g. player.$('selector')), I guess.

@gkatsev
Video.js member
gkatsev added a note

Yeah, I guess them leaking that way isn't completely terrible because it'll mostly be used for vjs-related things and not just people using videojs.$ for other stuff that they would just use jquery for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
misteroneill added some commits
@misteroneill misteroneill Add $ and $$ convenience methods 47fb2ea
@misteroneill misteroneill Reduce duplication in $ and $$ helpers 5ba13df
@misteroneill misteroneill Expose DOM helpers on videojs 22274e0
@misteroneill misteroneill Expose $ and $$ on component 9f45d67
@misteroneill misteroneill Use $ and $$ helpers where possible 262438c
@misteroneill misteroneill Rewrite className functions
I think it makes sense to use more powerful native APIs when they are
available. In this case, we use `classList` for all browsers but IE8 and
IE9.

Also, adds slightly more rigorous tests for these functions.
c5c645f
@misteroneill misteroneill Don't assume Error is thrown c039ec8
@misteroneill misteroneill Expose more methods. ef8d2f5
@misteroneill misteroneill Improve documentation comments. 8d7b965
@misteroneill misteroneill Add toggleElClass
- Also adds comprehensive tests.
- Exposes the function as `toggleClass` on `videojs`.
- Adds the `toggleClass` method to components.
a5f45fa
@misteroneill misteroneill Don't expose $ and $$ on videojs
a697a2c
@gkatsev
Video.js member

LGTM. Going to make another PR after this to fix the tracks JSON that I commented about above.

@gkatsev gkatsev closed this in f2fa8f8
@misteroneill misteroneill deleted the misteroneill:expose-dom-helpers branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.