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

videojs-standard #3459

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 0 additions & 47 deletions .jshintrc

This file was deleted.

21 changes: 8 additions & 13 deletions build/grunt.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,14 +114,6 @@ module.exports = function(grunt) {
build: ['build/temp/*'],
dist: ['dist/*']
},
jshint: {
src: {
src: ['src/js/**/*.js', 'Gruntfile.js', 'test/unit/**/*.js'],
options: {
jshintrc: '.jshintrc'
}
}
},
uglify: {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a temporary step. The final PR in this series will restore automatic linting capabilities, but for now we want them off.

options: {
sourceMap: true,
Expand Down Expand Up @@ -160,10 +152,6 @@ module.exports = function(grunt) {
skin: {
files: ['src/css/**/*'],
tasks: ['sass']
},
jshint: {
files: ['src/**/*', 'test/unit/**/*.js', 'Gruntfile.js'],
tasks: 'jshint'
}
},
connect: {
Expand Down Expand Up @@ -443,6 +431,14 @@ module.exports = function(grunt) {
src: ['build/temp/video.js']
}
}
},
shell: {
lint: {
command: 'vjsstandard',
options: {
preferLocal: true
}
}
}
});

Expand All @@ -455,7 +451,6 @@ module.exports = function(grunt) {
const buildDependents = [
'clean:build',

'jshint',
'browserify:build',
'exorcise:build',
'concat:novtt',
Expand Down
18 changes: 14 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"homepage": "http://videojs.com",
"author": "Steve Heffernan",
"scripts": {
"lint": "vjsstandard",
"test": "grunt test"
},
"repository": {
Expand Down Expand Up @@ -45,6 +46,7 @@
"css": "^2.2.0",
"es5-shim": "^4.1.3",
"es6-shim": "^0.35.1",
"ghooks": "^1.3.2",
"gkatsev-grunt-sass": "^1.1.1",
"grunt": "^0.4.4",
"grunt-aws-s3": "^0.12.1",
Expand All @@ -57,7 +59,6 @@
"grunt-contrib-connect": "~0.7.1",
"grunt-contrib-copy": "^0.8.0",
"grunt-contrib-cssmin": "~0.6.0",
"grunt-contrib-jshint": "~0.11.3",
"grunt-contrib-less": "~0.6.4",
"grunt-contrib-uglify": "^0.8.0",
"grunt-contrib-watch": "~0.1.4",
Expand All @@ -66,6 +67,7 @@
"grunt-fastly": "^0.1.3",
"grunt-github-releaser": "^0.1.17",
"grunt-karma": "^0.8.3",
"grunt-shell": "^1.3.0",
"grunt-version": "~0.3.0",
"grunt-videojs-languages": "0.0.4",
"grunt-zip": "0.10.2",
Expand All @@ -87,17 +89,25 @@
"sinon": "^1.16.1",
"time-grunt": "^1.1.1",
"uglify-js": "~2.3.6",
"videojs-doc-generator": "0.0.1"
"videojs-doc-generator": "0.0.1",
"videojs-standard": "^5.0.0"
},
"standard": {
"vjsstandard": {
"ignore": [
"**/Gruntfile.js",
"**/build/**",
"**/dist/**",
"**/docs/**",
"**/lang/**",
"**/sandbox/**",
"**/test/**"
"**/test/api/**",
"**/test/coverage/**",
"**/test/karma.conf.js"
]
},
"config": {
"ghooks": {
"pre-push": "npm run lint"
}
}
}
8 changes: 6 additions & 2 deletions src/js/base-styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,17 @@
import window from 'global/window';
import document from 'global/document';

if (window.VIDEOJS_NO_BASE_THEME) return;
if (window.VIDEOJS_NO_BASE_THEME) {
return;
}

const styles = '{{GENERATED_STYLES}}';

if (styles === '{{GENERATED'+'_STYLES}}');
// Don't think we need this as it's a noop?
// if (styles === '{{GENERATED'+'_STYLES}}');
Copy link
Member

Choose a reason for hiding this comment

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

I assume it was supposed to be something like

if (styles === '{{GENERATED'+'_STYLES'}}) {
  return;
}

So that if somehow the styles weren't inline, it won't bother with the rest.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, thoughts on what to do, since it's clearly never done that?

Copy link
Member

Choose a reason for hiding this comment

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

base-styles isn't even used anywhere anyway right now, so, feel free to just leave as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍


const styleNode = document.createElement('style');

styleNode.innerHTML = styles;

document.head.insertBefore(styleNode, document.head.firstChild);
28 changes: 16 additions & 12 deletions src/js/button.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@
*/
import ClickableComponent from './clickable-component.js';
import Component from './component';
import * as Events from './utils/events.js';
import * as Fn from './utils/fn.js';
import log from './utils/log.js';
Copy link
Member Author

Choose a reason for hiding this comment

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

There are removals like this scattered around: they were unused modules.

Note: if we're only trying to import the module, we should be doing something like import './utils/events'.

Copy link
Member

Choose a reason for hiding this comment

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

doesn't import './utils/events.js' put the exports into local scope? This is an issue for a "bag of functions" module.
import * Events from './utils/events.js' is more explicit in what we want.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tend to agree, but the linter doesn't like unused variable names.

Copy link
Member

Choose a reason for hiding this comment

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

if it isn't used then we shouldn't import it, sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

On the topic of using the import 'foo'; style, I don't think it puts the exports into local scope. I think it simply runs the module code. MDN says:

Import an entire module for side effects only, without importing any bindings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as require('foo.js');

Copy link
Member

Choose a reason for hiding this comment

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

Ah, interesting. I guess that's exactly what we want in player.js.

import document from 'global/document';
import assign from 'object.assign';

/**
Expand All @@ -32,7 +29,7 @@ class Button extends ClickableComponent {
* @return {Element}
* @method createEl
*/
createEl(tag='button', props={}, attributes={}) {
createEl(tag = 'button', props = {}, attributes = {}) {
props = assign({
className: this.buildCSSClass()
}, props);
Expand All @@ -53,11 +50,15 @@ class Button extends ClickableComponent {

// Add attributes for button element
attributes = assign({
type: 'button', // Necessary since the default button type is "submit"
'aria-live': 'polite' // let the screen reader user know that the text of the button may change

// Necessary since the default button type is "submit"
'type': 'button',

// let the screen reader user know that the text of the button may change
'aria-live': 'polite'
}, attributes);

let el = Component.prototype.createEl.call(this, tag, props, attributes);
const el = Component.prototype.createEl.call(this, tag, props, attributes);

this.createControlTextEl(el);

Expand All @@ -73,8 +74,9 @@ class Button extends ClickableComponent {
* @deprecated
* @method addChild
*/
addChild(child, options={}) {
let className = this.constructor.name;
addChild(child, options = {}) {
const className = this.constructor.name;

log.warn(`Adding an actionable (user controllable) child to a Button (${className}) is not supported; use a ClickableComponent instead.`);

// Avoid the error message generated by ClickableComponent's addChild method
Expand All @@ -87,13 +89,15 @@ class Button extends ClickableComponent {
* @method handleKeyPress
*/
handleKeyPress(event) {

// Ignore Space (32) or Enter (13) key operation, which is handled by the browser for a button.
if (event.which === 32 || event.which === 13) {
} else {
super.handleKeyPress(event); // Pass keypress handling up for unsupported keys
return;
}
}

// Pass keypress handling up for unsupported keys
super.handleKeyPress(event);
}
}

Component.registerComponent('Button', Button);
Expand Down
29 changes: 18 additions & 11 deletions src/js/clickable-component.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class ClickableComponent extends Component {
* @return {Element}
* @method createEl
*/
createEl(tag='div', props={}, attributes={}) {
createEl(tag = 'div', props = {}, attributes = {}) {
props = assign({
className: this.buildCSSClass(),
tabIndex: 0
Expand All @@ -51,11 +51,13 @@ class ClickableComponent extends Component {

// Add ARIA attributes for clickable element which is not a native HTML button
attributes = assign({
role: 'button',
'aria-live': 'polite' // let the screen reader user know that the text of the element may change
'role': 'button',

// let the screen reader user know that the text of the element may change
'aria-live': 'polite'
}, attributes);

let el = super.createEl(tag, props, attributes);
const el = super.createEl(tag, props, attributes);

this.createControlTextEl(el);

Expand Down Expand Up @@ -91,9 +93,11 @@ class ClickableComponent extends Component {
* @return {String}
* @method controlText
*/
controlText(text, el=this.el()) {
if (!text) return this.controlText_ || 'Need Text';

controlText(text, el = this.el()) {
if (!text) {
return this.controlText_ || 'Need Text';
}

const localizedText = this.localize(text);

this.controlText_ = text;
Expand Down Expand Up @@ -121,14 +125,14 @@ class ClickableComponent extends Component {
* @return {Component} The child component (created by this process if a string was used)
* @method addChild
*/
addChild(child, options={}) {
addChild(child, options = {}) {
// TODO: Fix adding an actionable child to a ClickableComponent; currently
// it will cause issues with assistive technology (e.g. screen readers)
// which support ARIA, since an element with role="button" cannot have
// actionable child elements.

//let className = this.constructor.name;
//log.warn(`Adding a child to a ClickableComponent (${className}) can cause issues with assistive technology which supports ARIA, since an element with role="button" cannot have actionable child elements.`);
// let className = this.constructor.name;
// log.warn(`Adding a child to a ClickableComponent (${className}) can cause issues with assistive technology which supports ARIA, since an element with role="button" cannot have actionable child elements.`);

return super.addChild(child, options);
}
Expand Down Expand Up @@ -179,12 +183,15 @@ class ClickableComponent extends Component {
* @method handleKeyPress
*/
handleKeyPress(event) {

// Support Space (32) or Enter (13) key operation to fire a click event
if (event.which === 32 || event.which === 13) {
event.preventDefault();
this.handleClick(event);
} else if (super.handleKeyPress) {
super.handleKeyPress(event); // Pass keypress handling up for unsupported keys

// Pass keypress handling up for unsupported keys
super.handleKeyPress(event);
}
}

Expand Down
Loading