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

Fixes #33 - Added eslint options for all the web extensions #38

Merged
merged 1 commit into from Sep 27, 2016

Conversation

Projects
None yet
2 participants
@akhilpandey95
Copy link
Member

commented Sep 6, 2016

Signed-off-by: AkhilHector akhilhector.1@gmail.com

@akhilpandey95

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2016

@akhilpandey95

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2016

  • Updated gitignore
  • Added eslint to package json
  • Added eslint and jpm to devDependencies
  • Test linted the firefox-webext/background.js
"homepage": "https://github.com/webcompat/webcompat-reporter-extensions#readme",
"devDependencies": {
"jpm": "^1.1.4",
"eslint": "^1.8.0",

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 7, 2016

Member

Can we go ahead and update to the latest eslint?

webcompat/webcompat.com#1172 just updated webcompat.com to eslint 3.4 -- it also has the rule changes needed there.

This comment has been minimized.

Copy link
@akhilpandey95

akhilpandey95 Sep 11, 2016

Author Member

done, made the changes accordingly

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 14, 2016

Member

Thanks!

(and thanks for being patient... traveling at the moment for a conference)

@akhilpandey95

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2016

Sure @miketaylr

@miketaylr

This comment has been minimized.

Copy link
Member

commented Sep 7, 2016

Hm, it seems like there's a lot of duplication here. I'm not sure what the best way to do this is... if we just have a single .eslintrc file that will make it so we don't have to update eslint options in multiple places.

@akhilpandey95

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2016

Yeah, i kind of had that in my mind. That's a good call ill create one eslintrc and use the same everywhere.

@akhilpandey95

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2016

@miketaylr good to go

.eslintrc Outdated
},
"extends": "eslint:recommended",
"globals": {
"jQuery": true,

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 14, 2016

Member

I should have caught this before... but these globals don't make sense for this repo. Can you remove and see what globals eslint complains about (and add those?).

"name": "chrome",
"version": "0.4.0",
"description": "Click to report an incompatible site at webcompat.com, and optionally upload a screenshot.",
"author": "Mike Taylor",

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 14, 2016

Member

You don't have to change this, but at this point "Mike Taylor" as author doesn't make much sense. ^_^

Maybe something like "webcompat.com team"?

@akhilpandey95

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2016

Noted all the things, im starting to think why did i not recheck on the globals. Thanks for the heads up. Will make changes and submit today.

@miketaylr

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

im starting to think why did i not recheck on the globals

heh, i didn't even notice for the first 2 reviews either. 🙈 🙉

@miketaylr
Copy link
Member

left a comment

  • remove irrelevan globals
  • replace Mike Taylor with "webcompat.com contributors"
@miketaylr

This comment has been minimized.

Copy link
Member

commented Sep 16, 2016

(sweet, just checking out the new review features)

@akhilpandey95
Copy link
Member Author

left a comment

  • Made eslint globals more relevant
  • Enabled es6 for template strings
  • Made changes to to firefox-webext/background.js
  • Changed the author in all package.json files
@@ -2,28 +2,28 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

var prefix = 'https://webcompat.com/?open=1&url=';
var screenshotData = '';
var prefix = `https://webcompat.com/?open=1&url=`;

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 19, 2016

Member

So, we don't want to change everything to a template literal, unless we're doing some kind of interpolation. Can you change everything back?

});


function reportIssue (tab) {
chrome.tabs.captureVisibleTab({format: 'png'}, function(res) {
function reportIssue() {

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 19, 2016

Member

Why did you remove the tab argument?

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 19, 2016

Member

(I'm guessing maybe eslint complains? It's nice to know that a tab object is being passed into reportIssue, IMO.)

chrome.tabs.executeScript({
code: 'window.postMessage("' + screenshotData + '", "*")'
code: `window.postMessage("` `${screenshotData}` `", "*")`

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 19, 2016

Member
code: `window.postMessage("` `${screenshotData}`  `", "*")`

why not just

code: `window.postMessage("${screenshotData}", "*")`
"homepage": "https://github.com/webcompat/webcompat-reporter-extensions#readme",
"devDependencies": {
"jpm": "^1.1.4",
"eslint": "^1.8.0",

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 19, 2016

Member

Did you forget to update to eslint 3.4?

var prefix = 'https://webcompat.com/?open=1&url=';
var screenshotData = '';
var reporterID = 'addon-reporter-firefox';
var prefix = `https://webcompat.com/?open=1&url=`;

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 19, 2016

Member

Same comments as above.

name: 'X-Reported-With',
value: `${reporterID}`
});
function addHeader(details) {

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 19, 2016

Member

Any particular reason to add a named function here?

@@ -2,23 +2,23 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

var prefix = 'https://webcompat.com/?open=1&url=';
var screenshotData = '';
var prefix = `https://webcompat.com/?open=1&url=`;

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 19, 2016

Member

Same comments as above.

AkhilHector
Issue #33 - Added eslint options for all the extensions, Updated giti…
…gnore

Signed-off-by: AkhilHector <akhilhector.1@gmail.com>
@miketaylr

This comment has been minimized.

Copy link
Member

commented Sep 27, 2016

Looks good! Only one minor issue w/ including jpm and web-ext in non-Firefox package.jsons -- but I can push a follow up commit there.

@miketaylr miketaylr merged commit ebc398d into webcompat:master Sep 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.