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

add a hasClass() method #1464

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@albell
Contributor

albell commented Aug 30, 2014

Two goals here:

First, the removeClass() wasn't adding spaces before and after element.className and the class. This could create weird false positives as we get into compound class names, like vjs-fluid, and vjs-fluid-4-3. This aligns the check across both methods.

Second, I think hasClass is going to be essential for testing, going forward.

add a hasClass() method
Two goals here:

First, the removeClass() wasn't adding spaces before and after element.className and the class. This could create weird false positives as we get into compound class names, like vjs-fluid, and vjs-fluid-4-3. This aligns the check across both methods.

Second, I think hasClass is going to be essential for testing, going forward.
@gkatsev

View changes

Show outdated Hide outdated src/js/lib.js
@@ -314,18 +328,19 @@ vjs.addClass = function(element, classToAdd){
vjs.removeClass = function(element, classToRemove){
var classNames, i;
if (element.className.indexOf(classToRemove) == -1) { return; }
if (vjs.hasClass(element, classToRemove)) {

This comment has been minimized.

@gkatsev

gkatsev Aug 30, 2014

Member

if you make this if check !hasClass, and then return, it will keep the rest of this function not indented, which is nice.

@gkatsev

gkatsev Aug 30, 2014

Member

if you make this if check !hasClass, and then return, it will keep the rest of this function not indented, which is nice.

@gkatsev

View changes

Show outdated Hide outdated src/js/lib.js
* @private
*/
vjs.hasClass = function(element, classToCheck){
if ((' ' + element.className + ' ').indexOf(' ' + classToCheck + ' ') == -1) {

This comment has been minimized.

@gkatsev

gkatsev Aug 30, 2014

Member

Though, really, we should just return the comparison:

return (' ' + element.className + ' ').indexOf(' ' + classToCheck + ' ') === -1;
@gkatsev

gkatsev Aug 30, 2014

Member

Though, really, we should just return the comparison:

return (' ' + element.className + ' ').indexOf(' ' + classToCheck + ' ') === -1;
@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 30, 2014

Member

Other than the two comments, LGTM.

Member

gkatsev commented Aug 30, 2014

Other than the two comments, LGTM.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 2, 2014

Member

Yeah, this looks good to me, thanks! If you want to make the updates @gkatsev mentioned, and add a quick test this should be ready to go.

Member

heff commented Sep 2, 2014

Yeah, this looks good to me, thanks! If you want to make the updates @gkatsev mentioned, and add a quick test this should be ready to go.

@heff heff added confirmed labels Sep 2, 2014

@heff

View changes

Show outdated Hide outdated src/js/component.js
* @return {vjs.Component}
*/
vjs.Component.prototype.hasClass = function(classToCheck){
vjs.hasClass(this.el_, classToCheck);

This comment has been minimized.

@heff

heff Sep 2, 2014

Member

Actually this should return the result of vjs.hasClass.

@heff

heff Sep 2, 2014

Member

Actually this should return the result of vjs.hasClass.

albell added some commits Sep 3, 2014

fix hasClass prototype on Component
I think I'm there now. Thx for your patience!
@albell

This comment has been minimized.

Show comment
Hide comment
@albell

albell Sep 3, 2014

Contributor

I could also rewrite some of the older tests to use hasClass(), too. The older tests incorrectly match a substring, e.g.

https://github.com/videojs/video.js/blob/master/test/unit/component.js#L184

but I think this works now as is.

Contributor

albell commented Sep 3, 2014

I could also rewrite some of the older tests to use hasClass(), too. The older tests incorrectly match a substring, e.g.

https://github.com/videojs/video.js/blob/master/test/unit/component.js#L184

but I think this works now as is.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 29, 2014

Member

Good to go for this week's release.

Member

heff commented Sep 29, 2014

Good to go for this week's release.

@heff heff closed this in fd181aa Sep 29, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 29, 2014

Member

Merged in. Thank you!

Member

heff commented Sep 29, 2014

Merged in. Thank you!

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