/addon-wr

WIP on less sublte effect. #24

Merged
merged 8 commits into from Dec 8, 2017

Conversation

Reviewers

@pdehaan pdehaan

@biancadanforth biancadanforth

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

gregglind commented Dec 8, 2017

KNONW ISSUE:

  1. The hover box takes a bit to disappear.
  2. The hover box flips over when the span does.

Possible answere:

  1. On hover handler for teh flipped spans, rather than pure css?
@gregglind
WIP on less sublte effect. 
KNONW ISSUE:
1.  The hover box takes a bit to disappear.
2.  The hover box flips over when the span does.

Possible answere:
1.  On hover handler for teh flipped spans, rather than pure css?
fed142f

@gregglind gregglind requested a review from biancadanforth Dec 8, 2017

TESTPLAN.md
- - ending is `notification-x`
+ `npm run eslint`
@pdehaan

pdehaan Dec 8, 2017

Contributor

I think we still need to fix the parent npm run lint uber-test, since that failed when I checked earlier.

@gregglind

gregglind Dec 8, 2017

Member

npm run eslint NOT npm run lint

@gregglind

gregglind Dec 8, 2017

Member

ripped them all out. that was a partial conversion.

TESTPLAN.md
+ 1. Preference FALSE :: no `X_1057` header.
+ - refresh https://www.whatismybrowser.com/detect/what-http-headers-is-my-browser-sending
+ - Confirm that `X_1057` is NOT in the list
+ - refresh XHOUSE
@pdehaan

pdehaan Dec 8, 2017

Contributor

What's XHOUSE here?

@gregglind

gregglind Dec 8, 2017

Member

removed.

addon/webextension/content-script.js
@@ -54,11 +59,18 @@ function findAndReplace(wordList) {
<br/><a href="${SUPPORTURL}" target="_blank", rel="noopener noreferrer">
@pdehaan

pdehaan Dec 8, 2017

Contributor

nit: There shouldn't be a rogue comma in the middle of the <a> tag.

@gregglind

gregglind Dec 8, 2017

Member

good catch!

addon/webextension/content-script.js
});
+ // between 1-5 seconds, flip them back, but keep the over. see #22
@pdehaan

pdehaan Dec 8, 2017

Contributor

Timing is always very hard. For example, unless the "Privacy" text appears "above the fold", it may be unlikely that the user will even see the effect. 2-6 seconds may not be enough time to read the content and scroll down to observe the effect if it's in a "Privacy Policy" footer link. 🤷‍♀️

addon/webextension/background.js
@@ -77,10 +77,20 @@ class PersistentPageModificationEffect {
display: inline-block;
}
+ .donotdelete-revert {
+ transform: ScaleY(1);
@pdehaan

pdehaan Dec 8, 2017

Contributor

nit: scaleY(1) (lowercase)

Not sure if there'd be a way to extract this CSS into an external file so we could validate it using stylelint or something, but now I'm getting super nit-pickety...
Currently I just copy the contents into an external file and lint that manually, but it's not foolproof, and I'm a pretty big fool.

@gregglind

gregglind Dec 8, 2017

Member

No time :)

gregglind added some commits Dec 8, 2017

@gregglind
added 300 ms timeout to the hover box. yes this is gross, but it's en… 
…ough.

Right answer is to do make sure the insertCSS is done before doing things
bf2bdfa
@gregglind
better fix, attempt to wait for CSS loading
a6674ed
addon/webextension/background.js
+
+ /* after revert, revert the tooltip */
+ .donotdelete-revert .donotdelete-tooltip {
+ transform: scaleY(1) translateY(-50%);
@biancadanforth

biancadanforth Dec 8, 2017

Collaborator

Please update these styles as we discussed to avoid the tooltip glitch:

.donotdelete-revert {
  transform: scaleY(1);
  transition: transform 1s ease-in;
}

/* after revert, revert the tooltip */
.donotdelete-revert .donotdelete-tooltip {
  transform: scaleY(1) translateY(-50%);
  transition: transform 1s ease-in;
}
@gregglind

gregglind Dec 8, 2017

Member

done

addon/webextension/background.js
+ .donotdelete-revert .donotdelete-tooltip {
+ transform: scaleY(1) translateY(-50%);
+ }
+
.donotdelete-tooltip {
@biancadanforth

biancadanforth Dec 8, 2017

Collaborator

Can you also add font-style: normal !important; under "Neutralizing styles" for this selector? Just saw the text italicized otherwise on New York Times' website.

addon/webextension/background.js
if (this.protocolIsApplicable(tab.url) && tab.status === "complete") {
- browser.tabs.insertCSS(id, this.CSS);
+ await browser.tabs.insertCSS(id, this.CSS);
@biancadanforth

biancadanforth Dec 8, 2017

Collaborator

Just wanted to note here that we would really need to programmatically load the content script for new tabs after the CSS is injected with tabs.executeScript to ensure the tooltip HTML is hidden before its injected into the DOM.

@gregglind

gregglind Dec 8, 2017

Member

Gross, but accurate.

@gregglind

gregglind Dec 8, 2017

Member

re #24

TESTPLAN.md
-3. UI functionality: 'X' button
+ `npm run build` makes an addon:
@pdehaan

pdehaan Dec 8, 2017 •
Edited 1 time

Contributor

Nit: indenting needs one more space.

@biancadanforth
Collaborator

biancadanforth commented Dec 8, 2017 •
Edited 2 times

For posterity, there is one known unresolved display bug for when parent elements to the match word (.donotdelete element) have overflow:hidden applied. This occurs on Google search page for "army" for example.

Since it's hard to anticipate all possible inherited styles; we may want to consider a different approach in the future than appending the hover element as a child element to the match word .donotdelete element.

One idea: attach a single hover element to <body> (and apply a neutralizing styles boilerplate to the element), and anchor it to the mouse location when the user hovers over a match word. In this case, since the element is a child of <body> rather than a potentially deeply nested unknown element, we can expect it to inherit a much smaller variety of styles, and therefore can hope to have much better control.

@biancadanforth biancadanforth merged commit f4fde15 into master Dec 8, 2017

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