/addon-wr

Fix a few ESLint issues #10

Closed
wants to merge 6 commits into
from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@pdehaan @gregglind @biancadanforth
@pdehaan
Contributor

pdehaan commented Dec 6, 2017

No description provided.

addon/webextension/.eslintrc.json
- "rules": {
- "no-console": "warn"
- }
-}
@pdehaan

pdehaan Dec 6, 2017

Contributor

For some reason, this file was preventing ESLint from loading the parent config. Not sure why.

@pdehaan

pdehaan Dec 6, 2017 •
Edited 1 time

Contributor

I think this can be resolved by moving the ESLint config file up a directory into /addon/:

"use strict";

module.exports = {
  env: {
    browser: true,
    es6: true,
    webextensions: true,
  },
};

I guess maybe ESLint only looks for config files up the tree as long as there are no gaps.

addon/webextension/content-script.js
}
+ if (document.querySelector("#wanted")) {
@pdehaan

pdehaan Dec 6, 2017

Contributor

@gregglind Is this kosher? Or should this be if (document.querySelector("#seen")) { ... }?

addon/webextension/background.js
* Returns true only if the URL's protocol is in APPLICABLE_PROTOCOLS.
*/
protocolIsApplicable(url) {
- var anchor = document.createElement('a');
+ var anchor = document.createElement("a");
@pdehaan

pdehaan Dec 6, 2017

Contributor

Not sure if we want to force let and const instead of using var... But that's probably a future conversation.

addon/webextension/background.js
- * Ensure that the CSS modification happens for all open and future tabs
- */
+ * Ensure that the CSS modification happens for all open and future tabs
+ */
insertCSSOnAllTabs() {
// When first loaded, add CSS for open tabs.
var gettingAllTabs = browser.tabs.query({});
@pdehaan

pdehaan Dec 6, 2017

Contributor 
/Users/pdehaan/dev/github/mozilla/addon-wr/addon/webextension/background.js
   80:5  error  Unexpected var, use let or const instead  no-var
  101:5  error  Unexpected var, use let or const instead  no-var

gregglind and others added some commits Dec 6, 2017

@gregglind
Merge pull request #8 from gregglind/nits-and-integration-1 
Fix nits and integration.  Still awaiting strings
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
4067b32
@gregglind
Merge branch 'master' of github.com:gregglind/addon-wr
0561db8
@biancadanforth
Add hover effect. Closes #9. 
I discovered why we were seeing multiple "span" elements wrapped around the same matching word -- we were looking for "div" and "p" elements to check, and frequently, a "div" would have nested "div"s and "p"s, so if a word was in one of the nested elements, it'd get wrapped multiple times. I have changed the `querySelectorAll` to look for "p", "h1", "h2" and "h3" now, which are not commonly nested within each other. A quick check on the Mozilla privacy page showed they are never nested within each other for that page.

Loose ends:
* There is currently a race condition with addon start-up between the background script and content scripts being loaded into existing tabs. The content script immediately sends a message to the background script to get the word list, sometimes before it is listening, throwing an error. Currently, this means some of the time on "web-ext run", the content script executes correctly, and somethings it throws and does nothing. This seems like a chicken and egg problem to me, so I would appreciate ideas.
* The page linked on the hover effect element currently opens in a new window, rather than a new tab. This is unusual behavior, since I am using an "a" element with target="_blank". Do we care?
* While the fix above for multiple wrapped "span" elements should help some with the performance issue (this addon is slowing down Firefox), we should check to see if we are still receiving the warning.
* The CSS for the hover effect adds quite a bit of CSS. Currently the position of the tooltip is to the right of the word, but there are styles in case we'd like to try "left", "bottom" and "top". Perhaps we should consider moving it to an external stylesheet now?
611590d
@gregglind
Merge pull request #12 from biancadanforth/9-add-hover-effect 
WIP - Add hover effect. Closes #9.
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
526535e
@pdehaan
Fix a few ESLint issues
cf3bb64
@pdehaan
A few more ESLint and package tweaks
This commit was signed with a verified signature.
@pdehaan pdehaan Peter deHaan
GPG key ID: F0FC6C01C6305097 Learn about signing commits
4c5991d

@pdehaan pdehaan closed this Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment