/video.js

Expose DOM Helpers #2754

Closed
wants to merge 11 commits into from
+706 −152

Labels

None yet

Milestone

No milestone

Assignees

No one assigned

2 participants

@misteroneill @gkatsev
@misteroneill
Video.js member
misteroneill commented Oct 28, 2015

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.

This was referenced Oct 28, 2015
Closed
Closed
Closed
@gkatsev gkatsev commented on the diff Nov 9, 2015
View full changes
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 Nov 9, 2015

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

@misteroneill
Video.js member
misteroneill added a note Nov 9, 2015

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 Nov 9, 2015
View full 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 Nov 9, 2015

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
misteroneill added a note Nov 9, 2015

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 Nov 9, 2015

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

@misteroneill
Video.js member
misteroneill added a note Nov 9, 2015

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 Nov 9, 2015

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 Oct 9, 2015
@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

All checks have passed

2 successful checks
Details continuous-integration/travis-ci/pr — The Travis CI build passed
Details coverage/coveralls — Coverage decreased (-0.02%) to 83.128%
a697a2c
@gkatsev
Video.js member
gkatsev commented Nov 9, 2015

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

@gkatsev gkatsev added a commit that closed this pull request Nov 9, 2015
@misteroneill misteroneill @misteroneill exposed DOM helpers. closes #2754 f2fa8f8
@gkatsev gkatsev closed this in f2fa8f8 Nov 9, 2015
@gkatsev gkatsev referenced this pull request Nov 9, 2015
Closed

5.0: consider exposing some dom.js methods #2683

@misteroneill misteroneill deleted the misteroneill:expose-dom-helpers branch Nov 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment