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

JSHint Uber PR #206

Merged
merged 10 commits into from
Sep 11, 2015
Merged

JSHint Uber PR #206

merged 10 commits into from
Sep 11, 2015

Conversation

gauntface
Copy link

I've gone through and made the changes to pass JSHint rules for the current set up and added it to the travis build process so that we should be able to see build status for PR's.

Problems / Questions:

  • The gulp files lint task is messy in terms of excludes and includes for the glob patterns because I had to skip html files by default and only reference them in the final build of the site. We could fix this by externalising the inline JS and then only run JSHint on the external files. We could still test for inlined JS on travis.
  • I've added gulp lint:watch for anyone to run while developing (Yes I would expect most problems to be caught by editor plugins - but it's a nice to have)
  • We don't currently use JSCS, should we add it in?

@jeffposnick
Copy link
Contributor

Awesome. Thanks, @gauntface! I'll take look a the PR and gulpfile structure, but regarding your last question:

JSCS would be very nice, but we would need to run it against the <script> tags in generated HTML files under _site/, and JSCS doesn't support <script> tag extraction: jscs-dev/node-jscs#764

(The reason why I originally went with JSHint instead of or ESLint was that JSHint natively supports <script> extraction, but https://github.com/BenoitZugmeyer/eslint-plugin-html could allow us to switch over to ESLint.)

@gauntface
Copy link
Author

If we extracted javascript from the pages, this wouldn't be an issue and adding JSCS to external JS files would still be better than nothing.

@jeffposnick
Copy link
Contributor

Would you want to rely on the Jekyll build process to stitch together the external .js files so that they resulted in inline <script> tags on the finished page? Or just reference them with <script src="...">?

I think there are some educational benefits to the current setup, where you can do a View Source... and see the JavaScript inline with the corresponding HTML. But maybe we could get creative and unconditionally include the entire contents of the external .js file wrapped in a <div class="highlight">? It could result in the equivalent of the "JavaScript Snippet" section of our existing samples.

As long as we don't sacrifice the experience of reading the samples and understanding how everything works in context for the benefit of better tooling, I'm on board.

@gauntface
Copy link
Author

Personally I would just reference the external file.

Using something to inline the snippet would be best of both worlds from that.

Best bit of this approach:

  • It's good practice - we should be demonstrating
  • We can have JSHint and JSCS on external JS files (We can still run JSHint over jekyll HTML if it's there, always going to happen on travis)
  • Might make adding JS snippets easier into the final page.

@jeffposnick
Copy link
Contributor

Cool. I've created #207 to track externalizing the JS files.

Would you rather I review this PR as-is, or would you like to modify it to include the JS externalization?

@gauntface
Copy link
Author

I think reviewing as is would be ideal. There may be strong votes against externalisation, plus this is a fairly big PR as it stands.

@@ -1,3 +1,5 @@
[![Build Status](https://travis-ci.org/gauntface/samples.svg?branch=gh-pages)](https://travis-ci.org/gauntface/samples)
Copy link
Contributor

Choose a reason for hiding this comment

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

gauntfaceGoogleChrome

@jeffposnick
Copy link
Contributor

Reviewed the current PR.

What do you think about wrapping the bundle exec jekyll build in a gulp task and setting it as a dependency for the lint task? If we're introducing a gulp dependency, we can use it to kick off all the build steps, and that would ensure that whenever lint is run, it's going against an up-to-date _site/.

Matt Gaunt added 3 commits September 4, 2015 10:29
* upstream/gh-pages:
  Review feedback.
  Review feedback.
  Added array method includes sample
  CSS.escape() sample
  Typo
  new.target sample
  Review feedback.
  Nicer HTTPS redirection for non-localhost.
  Added Event.isTrusted sample
  CSS motion path example
@gauntface
Copy link
Author

Updated to upstream gh-pages and addressed your concerns @jeffposnick to get something suitable for the repo.

Would ideally like to get something close to this in the repo so future samples are linted and then address externalising JS and simplifying the gulp.src rules in that PR.

@@ -1,3 +1,5 @@
[![Build Status](https://travis-ci.org/gauntface/samples.svg?branch=gh-pages)](https://travis-ci.org/GoogleChrome/samples)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's still a gauntface in there.

@jeffposnick
Copy link
Contributor

👍 other than the two small comments. Feel free to merge once those are addressed, and thanks!

gauntface pushed a commit that referenced this pull request Sep 11, 2015
@gauntface gauntface merged commit 6fbc941 into GoogleChrome:gh-pages Sep 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants