Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Strings part 1 #23
Conversation
gregglind
added some commits
Dec 7, 2017
| +truth | ||
| +debt | ||
| +cryptocurrency | ||
| +kernel panic |
pdehaan
Dec 8, 2017
Contributor
Can somebody explain this to me? Our regex splits on \s+, so wouldn't this parse as two separate words anyways? Or should we update the regexp to split on \n?
gregglind
Dec 8, 2017
Member
It will go on both words as single words. I decided not to be more clever than that.
| @@ -18,6 +22,8 @@ myPort.onMessage.addListener(function(m) { | ||
| } | ||
| }); | ||
| +const SUPPORTURL = `https://support.mozilla.org/en-US/lookingglass` |
pdehaan
Dec 8, 2017
Contributor
This currently 404s, but I trust will be live in time since I see how it's used below.
(nit: Also lacks a semi-colon and can probably use double quotes instead of template backticks)
| +truth | ||
| +debt | ||
| +cryptocurrency | ||
| +kernel panic |
biancadanforth
Dec 8, 2017
Collaborator
Is this a phrase? If so, we may need to use Array instead of splitting a string?
| "https://www.whatismybrowser.com/detect/*" | ||
| - ], | ||
| + ], |
pdehaan
Dec 8, 2017
•
Edited 1 time
-
pdehaan
Dec 8, 2017
Contributor
-
Dec 8, 2017
nit: Indenting is off here.
Not sure if ESLint is not catching these, or we don't have Circle-CI set up to run linting/tests for each PR.
| + Can you trust your perceptions? | ||
| + You chose this... a reminder of the forces at work in your world. | ||
| + If you no longer wish to peer through the looking glass, you can | ||
| + <br/><a href="${SUPPORTURL}" target="_blank"> |
pdehaan
Dec 8, 2017
Contributor
Re: target="_blank", not sure if we want to add rel="noopener noreferrer", per https://www.jitbit.com/alexblog/256-targetblank---the-most-underestimated-vulnerability-ever/
| @@ -12,7 +12,7 @@ XPCOMUtils.defineLazyModuleGetter(this, "LegacyExtensionsUtils", | ||
| const prefs = Services.prefs; | ||
| // our pref, set it with default false, if it's not there. | ||
| -const PREFNAME = "browser.display.truth"; | ||
| +const PREFNAME = "extensions.pug.lookingglass"; |
biancadanforth
Dec 8, 2017
Collaborator
What's "PUG"? In the soccer world I came from, that's "pick-up-group"; don't think that's it though.
| -const XHEADERSITES = ['<all_urls>']; | ||
| -const XHEADERNAME = 'dontdeleteme'; | ||
| -const XHEADERVALUE = '1057' | ||
| +const WORDS = |
| @@ -41,11 +47,12 @@ function findAndReplace(wordList) { | ||
| document.querySelectorAll(".donotdelete").forEach((node) => { | ||
| const hoverEle = document.createElement("span"); | ||
| hoverEle.innerHTML = ` |
pdehaan
Dec 8, 2017
Contributor
I'm getting this locally from ESLint:
/Users/pdehaan/dev/github/mozilla/addon-wr/addon/webextension/content-script.js
49:5 error Unsafe assignment to innerHTML no-unsanitized/property
108:10 warning Expected a conditional expression and instead saw an assignment no-cond-assign
112:11 warning Expected a conditional expression and instead saw an assignment no-cond-assign
✖ 3 problems (1 error, 2 warnings)
It was an .innerHTML before, so doesn't look like this is a new problem. Not sure if we just want to slap an
// eslint-disable-next-line no-unsanitized/property on the previous line to appease the ESLint gods (since we know what we're doing and accept the risks).
| @@ -43,7 +43,7 @@ | ||
| "path": "^0.12.7", |
pdehaan
Dec 8, 2017
•
Edited 1 time
-
pdehaan
Dec 8, 2017
Contributor
-
Dec 8, 2017
Unrelated, but this certainly shouldn't be in devDependencies since we're probably wanting the native Node path module.
I'll take a peek at the upstream shield template, but we should
UPDATE: Upstream PR at mozilla/shield-studies-addon-template#41
gregglind
added some commits
Dec 8, 2017
| hoverEle.innerHTML = ` | ||
| Can you trust your perceptions? | ||
| You chose this... a reminder of the forces at work in your world. | ||
| If you no longer wish to peer through the looking glass, you can | ||
| - <br/><a href="${SUPPORTURL}" target="_blank"> | ||
| + <br/><a href="${SUPPORTURL}" target="_blank", rel="noopener noreferrer"> |
biancadanforth
reviewed
Dec 8, 2017
Code doesn't compile as is; need to fix two syntax issues, as commented.
| @@ -18,6 +22,8 @@ myPort.onMessage.addListener(function(m) { | ||
| } | ||
| }); | ||
| +const SUPPORTURL = "https://support.mozilla.org/kb/lookingglass"; |
biancadanforth
Dec 8, 2017
Collaborator
This page says "test - 12817 This article doesn't have approved content yet."; so it's not ready yet, but I assume it's the right page?
| + // skip scripts and styles | ||
| + const tag = node.parentElement.tagName; | ||
| + const skipped_tags = ["STYLE", "SCRIPT", "CANVAS", "SVG"] | ||
| + if skipped_tags.includes(tag) { |
biancadanforth
Dec 8, 2017
Collaborator
This is invalid; need to add a parentheses around the condition statement for multiline if statements.
| } | ||
| + | ||
| + return NodeFilter.FILTER_ACCEPT; | ||
| }, |
biancadanforth
Dec 8, 2017
Collaborator
Missing another curly brace here; otherwise you get a parsing error on null.
This was referenced Dec 8, 2017
gregglind
added some commits
Dec 8, 2017
gregglind commentedDec 8, 2017
No description provided.