Add JSHint support and enable strict mode #379
Conversation
|
On a side note, I face the same issue I describe in #378 in this branch too :/ EDIT: correction, the device navbar shows up and works, the device image isn't updated with the component used. |
|
PR updated. I added a small TODO in the description for the things we need to sort. |
| src: 'js/*.js' | ||
| }, | ||
| docs: { | ||
| src: ['docs/assets/js/docs.js', 'docs/assets/js/fingerblast.js'] |
There was a problem hiding this comment.
can/should we do something like: docs/assets/js/*.js?
There was a problem hiding this comment.
Yes, but that folder might have more files like docs.min.js (see the uglify PR)
|
Let's stay consistent with the bootstrap jshintrc. What do you think about that? |
|
At the moment, it makes more sense to use the options I used. Semicolons are already being used in most of the codebase; the rest are similar to Bootstrap except for I'm personally used to those 2 and I find Bootstrap's style a little weird, but it all comes down to one's personal taste. :) So if everyone wants to use the same options as Bootstrap that's ok, we can bring it on par. PS. There's still a Bootstrap PR twbs/bootstrap#12853, but I'll leave this for when we merge it. |
|
I'm +1 on not slavishly following Bootstrap's jshintrc, and sticking with a more conventional code style. |
| { | ||
| "boss" : true, | ||
| "browser" : true, | ||
| "debug" : true, |
There was a problem hiding this comment.
Are debug and devel actually necessary to make the code pass?
There was a problem hiding this comment.
No, I think they are just remnants. I'll double check later.
There was a problem hiding this comment.
We might be able to get around it by using window.alert() instead of alert(), etc.
There was a problem hiding this comment.
I don't mind if you want to do that yourself but I think it should be part of another PR; this one has already grown big.
From: Chris Rebert notifications@github.com
To: twbs/ratchet ratchet@noreply.github.com
Cc: XhmikosR xhmikosr@yahoo.com
Sent: Monday, March 3, 2014 8:53 PM
Subject: Re: [ratchet] Add JSHint support and enable strict mode (#379)In js/.jshintrc:
@@ -0,0 +1,17 @@
+{
- "boss" : true,
- "browser" : true,
- "debug" : true,
We might be able to get around it by using window.alert() instead of alert(), etc.
—
Reply to this email directly or view it on GitHub.
|
So, how would folks feel about |
|
I like using curly braces. |
|
Don't feel bound to Bootstrap's conventions. The two libraries are separate and don't need 100% parity for the time being. We can work to improve that in future major releases :). |
|
This PR is ready, and everything works fine (like before, at least on desktop). I'll wait for the final comments on the JSHint's options, and then rebase and merge if everyone agrees. |
|
+1 |
|
I'll wait for the others too and adapt the PR later. I agree with all those you mention but since they yield many warnings I skipped them IIRC. Now, the situation might be better, I'll need to double check. |
|
All done @cvrebert. Let's stop adding stuff so that we finally merge this 👯 |
|
So, what should I do with this guys? I'll still need to rebase the branch to squash a few patches if they stay as they are now. |
|
Cool. What you've got is 👍 for me. |
Enabled: * `immed` * `newcap` * `latedef` Removed: * `boss` * `laxbreak` * `validthis` Also move `bitwise` to fingerblast.js since that's where it's only needed.
|
👍 ❤️ |
Add JSHint support and enable strict mode
|
Hopefully, nothing broke :) |
|
🙏 |
Fixes #280. Fixes #285.
TODO:
globaldirectivesbitwiseandimmedboss/CC @connor @connors @cvrebert @mdo