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.

Sign up for GitHub

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

Jump to bottom

Setup linter for HTML #5195

Merged
merged 41 commits into from Feb 3, 2018
Merged

Setup linter for HTML #5195

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
3ff10ac
Add htmllint
loicbourgois Jan 27, 2018
13cd5b0
Add htmllint
loicbourgois Jan 27, 2018
fddb06f
Setup html plugin for eslint
loicbourgois Jan 28, 2018
6f821d7
Add option to lint script tags
loicbourgois Jan 28, 2018
83822eb
Rename option
loicbourgois Jan 28, 2018
fd5232a
Convert spaces to tabs
loicbourgois Jan 28, 2018
7cfcf5a
Setup eslint fix
loicbourgois Jan 28, 2018
1b591b1
setup
loicbourgois Jan 28, 2018
9f130a2
Autofix eslint
loicbourgois Jan 28, 2018
c640e69
Rollback before autofix
loicbourgois Jan 28, 2018
098c133
Remove most errors
loicbourgois Jan 28, 2018
71792f1
Correct introduced errors
loicbourgois Jan 28, 2018
70822cf
Check samples
loicbourgois Jan 28, 2018
f8f119a
Fix lint
loicbourgois Jan 28, 2018
8183537
Add TODO comments
loicbourgois Jan 28, 2018
0171194
revert window.Samples
loicbourgois Jan 30, 2018
f588414
Move config to samples/.eslintrc.yml
loicbourgois Jan 30, 2018
599fd09
Enforce camelcase rule
loicbourgois Jan 30, 2018
c3c0bea
Remove gulp-html-validator and refactor gulpfile.js
loicbourgois Jan 30, 2018
066b3ae
Correct typo
loicbourgois Jan 30, 2018
31d6571
Move htmllint config to .json file
loicbourgois Jan 30, 2018
0940fdd
SteppedLines
loicbourgois Jan 30, 2018
8378328
Revert "Move htmllint config to .json file"
loicbourgois Jan 30, 2018
adbfb4d
Introduce error
loicbourgois Jan 30, 2018
27fa1a7
Revert "Revert "Move htmllint config to .json file""
loicbourgois Jan 30, 2018
6f96837
Fix src for lintHtmlTask()
loicbourgois Jan 30, 2018
9d2d092
Revert "Introduce error"
loicbourgois Jan 30, 2018
8ac4864
Use default config file for htmllint
loicbourgois Jan 31, 2018
ccd9cab
Fail on error
loicbourgois Jan 31, 2018
1fc07d2
Remove error
loicbourgois Feb 1, 2018
8e4c315
Remove unecessary comment
loicbourgois Feb 1, 2018
4675e85
Add link to issue for add data bug
loicbourgois Feb 1, 2018
9b59b55
Clean gulpfile
loicbourgois Feb 1, 2018
9bb89f4
Revert to inline event handler
loicbourgois Feb 1, 2018
3746bad
Disable no-unused-vars for inline events
loicbourgois Feb 1, 2018
261085f
Revert "Disable no-unused-vars for inline events"
loicbourgois Feb 1, 2018
1474880
Indentation
loicbourgois Feb 1, 2018
57fc01c
Remove old htmlTask
loicbourgois Feb 1, 2018
4a2ff8c
Fix package.json
loicbourgois Feb 1, 2018
03321b0
Fix plugin id
loicbourgois Feb 1, 2018
58db09d
eslint-disable-next-line no-unused-vars
loicbourgois Feb 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .eslintrc.yml
View file
Open in desktop
Expand Up @@ -3,3 +3,5 @@ extends: chartjs
env:
browser: true
node: true

plugins: ['html']
10 changes: 10 additions & 0 deletions .htmllintrc.js
View file
Open in desktop
@@ -0,0 +1,10 @@
module.exports = {
Copy link
Member

Choose a reason for hiding this comment

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

Does htmllint support YAML config? It's usually more readable and easy to manipulate but also consistent with our other config files.

'rules': {
'indent-style': 'tabs',
'attr-quote-style': 'double',
'spec-char-escape': false,
'attr-bans': ['align', 'background', 'bgcolor', 'border', 'frameborder', 'longdesc', 'marginwidth', 'marginheight', 'scrolling', 'syle'/*, 'width'*/],
Copy link
Member

Choose a reason for hiding this comment

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

I think there is a typo to syle but do we really need these explicit bans?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took the default option and commented the attributes triggering errors.
See https://github.com/htmllint/htmllint/wiki/Options#attr-bans

Copy link
Member

Choose a reason for hiding this comment

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

I see, then you can remove the 2 last values ('syle' which was initially 'style' and 'width')

'tag-bans': [/*'style', */'b', 'i'],
'id-class-style': false,
}
}
30 changes: 26 additions & 4 deletions gulpfile.js
View file
Open in desktop
Expand Up @@ -20,7 +20,9 @@ var collapse = require('bundle-collapser/plugin');
var yargs = require('yargs');
var path = require('path');
var fs = require('fs');
var htmllint = require('gulp-htmllint');
var package = require('./package.json');
var htmllintOptions = require('./.htmllintrc.js');

var argv = yargs
.option('force-output', {default: false})
Expand Down Expand Up @@ -50,8 +52,9 @@ gulp.task('build', buildTask);
gulp.task('package', packageTask);
gulp.task('watch', watchTask);
gulp.task('lint', lintTask);
gulp.task('lint-html', lintHtmlTask);
Copy link
Member

Choose a reason for hiding this comment

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

This task should be ran as a dependency of the lint task, not test

gulp.task('docs', docsTask);
gulp.task('test', ['lint', 'validHTML', 'unittest']);
Copy link
Member

Choose a reason for hiding this comment

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

We should also remove the validHtml associated function

gulp.task('test', ['lint', 'lint-html', 'validHTML', 'unittest']);
Copy link
Member

Choose a reason for hiding this comment

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

I think htmllint do the same job as gulp-html-validator (unmaintained?), so we should remove the 'validHTML' task and associated node dependencies in package.json.

gulp.task('size', ['library-size', 'module-sizes']);
gulp.task('server', serverTask);
gulp.task('validHTML', validHTMLTask);
Expand Down Expand Up @@ -157,7 +160,8 @@ function lintTask() {
var files = [
'samples/**/*.js',
'src/**/*.js',
'test/**/*.js'
'test/**/*.js',
'./samples/**/*.html'
Copy link
Member

Choose a reason for hiding this comment

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

For consistency:

var files = [
    'samples/**/*.html'
    'samples/**/*.js',
    'src/**/*.js',
    'test/**/*.js'
]

];

// NOTE(SB) codeclimate has 'complexity' and 'max-statements' eslint rules way too strict
Expand All @@ -166,8 +170,21 @@ function lintTask() {
var options = {
rules: {
'complexity': [1, 10],
'max-statements': [1, 30]
}
'max-statements': [1, 30],
'no-new': 0, // TODO : is this allowed ?
Copy link
Member

Choose a reason for hiding this comment

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

It's not allow to call new without storing the result. We should fix these cases instead of disabling this rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried var chart = new Chart(...) but then it triggers an error because chart is not used.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, let's disable no-new then

//TODO : see
// /Chart.js/samples/charts/area/line-datasets.html
// 125:5 error Identifier 'samples_filler_analyser' is not in camel case camelcase
// /Chart.js/samples/charts/area/radar.html
// 103:5 error Identifier 'samples_filler_analyser' is not in camel case camelcase
'camelcase': 0,
Copy link
Member

Choose a reason for hiding this comment

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

Does it pass with 'samples-filler-analyser'? else this rule should be moved in samples/.eslintrc.yml

},
globals: [ // TODO : do we need to use window.randomScalingFactor everytime ?
'$',
'Chart',
'moment',
'randomScalingFactor',
]
Copy link
Member

Choose a reason for hiding this comment

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

These globals should not be enabled for the whole repository but scoped to the samples directory in its own samples/.eslintrc.yml.

};

return gulp.src(files)
Expand All @@ -176,6 +193,11 @@ function lintTask() {
.pipe(eslint.failAfterError());
}

function lintHtmlTask() {
return gulp.src('./samples/**/*.html')
.pipe(htmllint(htmllintOptions));
Copy link
Member

Choose a reason for hiding this comment

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

No need to require htmllintOptions, I think this should work:

  .pipe(htmllint({
    config: '.htmllint.js' // or .yml if supported
  }));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the source, gulp-htmllint only supports proper JSON
https://github.com/yvanavermaet/gulp-htmllint/blob/master/src/index.js#L20

}

function docsTask(done) {
const script = require.resolve('gitbook-cli/bin/gitbook.js');
const cmd = process.execPath;
Expand Down
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -17,13 +17,15 @@
"coveralls": "^3.0.0",
"eslint": "^4.9.0",
"eslint-config-chartjs": "^0.1.0",
"eslint-plugin-html": "^4.0.2",
"gitbook-cli": "^2.3.2",
"gulp": "3.9.x",
"gulp-concat": "~2.6.x",
"gulp-connect": "~5.0.0",
"gulp-eslint": "^4.0.0",
"gulp-file": "^0.3.0",
"gulp-html-validator": "^0.0.5",
Copy link
Member

Choose a reason for hiding this comment

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

Should be removed (see previous comment)

"gulp-htmllint": "0.0.15",
Copy link
Member

Choose a reason for hiding this comment

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

"^0.0.15"

"gulp-insert": "~0.5.0",
"gulp-replace": "^0.6.1",
"gulp-size": "~2.1.0",
Expand Down
226 changes: 113 additions & 113 deletions samples/advanced/data-labelling.html
View file
Open in desktop
Expand Up @@ -3,130 +3,130 @@
<html>

<head>
<title>Labelling Data Points</title>
<script src="../../dist/Chart.bundle.js"></script>
<script src="../utils.js"></script>
<style>
canvas {
-moz-user-select: none;
-webkit-user-select: none;
-ms-user-select: none;
}
</style>
<title>Labelling Data Points</title>
Copy link
Member

Choose a reason for hiding this comment

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

All files are now indented with 2 tabs instead of 1 for the HTML part

image

<script src="../../dist/Chart.bundle.js"></script>
<script src="../utils.js"></script>
<style>
canvas {
-moz-user-select: none;
-webkit-user-select: none;
-ms-user-select: none;
}
</style>
</head>

<body>
<div style="width: 75%">
<canvas id="canvas"></canvas>
</div>
<button id="randomizeData">Randomize Data</button>
<script>
var color = Chart.helpers.color;
var barChartData = {
labels: ["January", "February", "March", "April", "May", "June", "July"],
datasets: [{
type: 'bar',
label: 'Dataset 1',
backgroundColor: color(window.chartColors.red).alpha(0.2).rgbString(),
borderColor: window.chartColors.red,
data: [
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor()
]
}, {
type: 'line',
label: 'Dataset 2',
backgroundColor: color(window.chartColors.blue).alpha(0.2).rgbString(),
borderColor: window.chartColors.blue,
data: [
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor()
]
}, {
type: 'bar',
label: 'Dataset 3',
backgroundColor: color(window.chartColors.green).alpha(0.2).rgbString(),
borderColor: window.chartColors.green,
data: [
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor()
]
}]
};
<div style="width: 75%">
<canvas id="canvas"></canvas>
</div>
<button id="randomizeData">Randomize Data</button>
<script>
var color = Chart.helpers.color;
var barChartData = {
labels: ['January', 'February', 'March', 'April', 'May', 'June', 'July'],
datasets: [{
type: 'bar',
label: 'Dataset 1',
backgroundColor: color(window.chartColors.red).alpha(0.2).rgbString(),
borderColor: window.chartColors.red,
data: [
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor()
]
}, {
type: 'line',
label: 'Dataset 2',
backgroundColor: color(window.chartColors.blue).alpha(0.2).rgbString(),
borderColor: window.chartColors.blue,
data: [
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor()
]
}, {
type: 'bar',
label: 'Dataset 3',
backgroundColor: color(window.chartColors.green).alpha(0.2).rgbString(),
borderColor: window.chartColors.green,
data: [
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor()
]
}]
};

// Define a plugin to provide data labels
Chart.plugins.register({
afterDatasetsDraw: function(chart, easing) {
// To only draw at the end of animation, check for easing === 1
var ctx = chart.ctx;
// Define a plugin to provide data labels
Chart.plugins.register({
afterDatasetsDraw: function(chart/* , easing */) { // TODO : remove easing ?
// To only draw at the end of animation, check for easing === 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment and code would be out of sync

Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove both the variable and the comment

var ctx = chart.ctx;

chart.data.datasets.forEach(function (dataset, i) {
var meta = chart.getDatasetMeta(i);
if (!meta.hidden) {
meta.data.forEach(function(element, index) {
// Draw the text in black, with the specified font
ctx.fillStyle = 'rgb(0, 0, 0)';
chart.data.datasets.forEach(function(dataset, i) {
var meta = chart.getDatasetMeta(i);
if (!meta.hidden) {
meta.data.forEach(function(element, index) {
// Draw the text in black, with the specified font
ctx.fillStyle = 'rgb(0, 0, 0)';

var fontSize = 16;
var fontStyle = 'normal';
var fontFamily = 'Helvetica Neue';
ctx.font = Chart.helpers.fontString(fontSize, fontStyle, fontFamily);
var fontSize = 16;
var fontStyle = 'normal';
var fontFamily = 'Helvetica Neue';
ctx.font = Chart.helpers.fontString(fontSize, fontStyle, fontFamily);

// Just naively convert to string for now
var dataString = dataset.data[index].toString();
// Just naively convert to string for now
var dataString = dataset.data[index].toString();

// Make sure alignment settings are correct
ctx.textAlign = 'center';
ctx.textBaseline = 'middle';
// Make sure alignment settings are correct
ctx.textAlign = 'center';
ctx.textBaseline = 'middle';

var padding = 5;
var position = element.tooltipPosition();
ctx.fillText(dataString, position.x, position.y - (fontSize / 2) - padding);
});
}
});
}
});
var padding = 5;
var position = element.tooltipPosition();
ctx.fillText(dataString, position.x, position.y - (fontSize / 2) - padding);
});
}
});
}
});

window.onload = function() {
var ctx = document.getElementById("canvas").getContext("2d");
window.myBar = new Chart(ctx, {
type: 'bar',
data: barChartData,
options: {
responsive: true,
title: {
display: true,
text: 'Chart.js Combo Bar Line Chart'
},
}
});
};
window.onload = function() {
var ctx = document.getElementById('canvas').getContext('2d');
window.myBar = new Chart(ctx, {
type: 'bar',
data: barChartData,
options: {
responsive: true,
title: {
display: true,
text: 'Chart.js Combo Bar Line Chart'
},
}
});
};

document.getElementById('randomizeData').addEventListener('click', function() {
barChartData.datasets.forEach(function(dataset) {
dataset.data = dataset.data.map(function() {
return randomScalingFactor();
})
});
window.myBar.update();
});
</script>
document.getElementById('randomizeData').addEventListener('click', function() {
barChartData.datasets.forEach(function(dataset) {
dataset.data = dataset.data.map(function() {
return randomScalingFactor();
});
});
window.myBar.update();
});
</script>
</body>

</html>