WIP: Search Injection #320

digi0ps · 2018-02-24T12:19:10Z

Fixes #281

Implemented injecting Memex results along with Google Search results.
I've created a new module in src/search-injection where the content_script resides.
The content script:

Matches the URL against a search engine's search URL
If the regex produces a match, retrieves the query parameter from the URL
Opens a two way connection to the background search script and sends the query
The results ( limited to 3 ) are then embedded in a React component and rendered on the page.

TODOs:

blackforestboi · 2018-02-24T13:10:10Z

Wow great that you got it running so quickly!

I just tested it and it does not work, showing me the error:

If i search via the regular search overview it gives me back the results.

digi0ps · 2018-02-24T14:36:59Z

Ah snap, that is because of the DOM element which is used to append the results hasn't been loaded on the page yet. ( Temporary fix: Try refreshing the page ) Am working on a proper fix for that. @oliversauter

blackforestboi · 2018-02-28T15:12:11Z

Just gave it a test and works wonderfully now!

Will leave some design notes in the issue itself, because now I have the designs for it :)

digi0ps · 2018-03-11T17:19:49Z

@oliversauter Check it out now. Prepended the css and can change positions. Have some UI tweaks left to do.

blackforestboi · 2018-03-11T21:55:41Z

Hey Sriram, thanks for updating your PR!

Just checked on it, still seem the same issue with the loading, and some others I uncovered:

it still shows the unstyled version when loading, resulting in an unsexy glitch.
This is a screenshot of it:
when hovering over "See all results" it should show the glove cursor
the very first time clicking "change position of memex" it does not work, only the second time. (could also reproduce it sometimes randomly that the first try does not work)
the settings icon is cut off when results are on the right.
Change Designs to the mockups of Jean Baptiste. See here.
we should definitely write tests for the major functionalities
when the results are on the right, we can show 5 results.
make sure the title overflows and not breaks into a second line, if it is too long.

digi0ps · 2018-03-19T08:04:42Z

src/search-injection/constants.js

@@ -18,7 +18,7 @@ export const SEARCH_ENGINES = {
    google: {
        regex: /(http[s]?:\/\/)?(www.)?google[.\w]+\/search\?.*/,
        container: {
-            above: 'center_col',
+            above: 'res',


Appending to res makes Memex Container go below ads. Did you see that?

changed it again, thanks for the headsup!

poltak

This is pretty awesome work @digi0ps and @oliversauter ! Code is pretty impressive. I built and tried it out, and my experience using it was really good. Very smooth, and cool how you can move it around.

I would postpone the screenshots for now. In the current index it is very painful to get them as you need to bring in Pouch and fetch the data for each result. In the new index, they are returned with the rest of the search results, so it will just be a matter of rendering them straight in a <img> somewhere once we merge that in

poltak · 2018-03-20T05:49:26Z

src/background.js

@@ -44,6 +45,13 @@ async function openOverview() {
    }
 }

+const openOverviewURL = url => chrome.tabs.create({ url })


Everywhere you've used the chrome global, replace it with browser for consistency with the rest of codebase. They provide similar APIs, although we're using the web ext polyfill (assigned to browser) as it provides more consistency across browsers and uses Promises for async stuff

Also, with the openOverviewURL function, why does it differ from openOptionsURL function in taking an absolute URL rather than just the path? I think you could simplify it a lot by just hardcoding the overview URL in there and having a query arg to append to it. Then content script side just needs to send the query; no need to know what the URL is.

Yeah right. I didn't know why I did that .-.
I will update it!

poltak · 2018-03-20T05:59:36Z

src/options/settings/index.js

+import SearchInjection from './components/SearchInjection'
+
+const Settings = () => (
+    <Wrapper>


Don't think <Wrapper> should be needed here. This is a "noop" component; it renders nothing to the real DOM. Only used as a helper to make some JSX code neater. And if you can remove this, this whole module is just const Settings = () => <SearchInjection /> - so probably unnecessary.

Instead what you could do is separate the view parts of SearchInjection (everything in the render() method) into it's own dumb component and just have the other state-interacting methods within a SearchInjectionContainer class (render() would simply just render the dumb component and pass in the needed props). This is the general container-view pattern in React.

Oooh. I thought wrapper was for the UI. Great second point.
Will this be good: I do all the state related logic in Settings component and pass it to the SearchInjection component?

poltak · 2018-03-20T06:01:45Z

src/search-injection/components/Dropdown.js

+
+const openSettings = () => {
+    const message = {
+        action: 'openOptionsURL',


It would be good to define these action names somewhere which is imported by both this content script code and the corresponding listener in the background script code. Maybe the search-injection/constants module?

poltak · 2018-03-20T06:08:22Z

src/search-injection/components/Results.js

+    }
+
+    toggleDropDown() {
+        this.setState(state => ({


You're using both reducer and object versions of this.setState; it would be good to be consistent with which you use. In the rest of the code base we're trying to use the reducer (function) taking version. Really important to note though, if you use the reducer version like here: the returned value of the callback will be the new state.

this.setState(state => ({ dropdown: !state.dropdown }))

The above will unset all the other state keys, like hideResults, meaing they will be undefined whenever they get accessed via this.state. That may be your intention here though, but it's often a gotcha with this.setState.

The following would ensure dropdown state is updated, and all other state keys are left alone:

this.setState(state => ({ ...state, dropdown: !state.dropdown }))

poltak · 2018-03-20T06:09:43Z

src/search-injection/components/Results.js

+            return null
+        }
+        return (
+            <div


Again, good to move this to its own view module. Separate the state interaction and views

poltak · 2018-03-20T06:16:37Z

src/search-injection/content_script.js

+
+    // Append content_script.css to the document
+    const cssFile = browser.extension.getURL('/content_script.css')
+    appendCss(cssFile)


Seems a bit odd to have the appendCss logic - which prepends (possible naming issue) a <script> tag to the DOM - in the search-injection/utils module while the other DOM stuff is in this module. Maybe you could move appendCss and maybe also have a appendReactRoot function to their own search-injection/dom module or something. Just a code-layout suggestion; the actual code seems very well done.

prepend was a very recent change, so forgive me on that. It injects CSS into the page, so I guess injectCSS would fit it. And that's a nice idea, I will do it.

poltak · 2018-03-20T06:34:38Z

src/search-injection/content_script.js

+        ReactDOM.render(
+            <Results
+                results={results.slice(0, limit)}
+                len={results.length}


results.length will always be <=10 because of pagination. Instead, where you call handleRender, you can use payload.searchResult.totalCount to get the real number (maybe just pass in the entire payload.searchResult and pull out the needed args: const handleRender = ({ docs, totalCount }) => { )

Ah, I was planning to look into this issue, as it never showed me a length greater than 10.
But I don't think the there is a totalCount attribute in the payload received.

poltak · 2018-03-20T13:13:25Z

That sounds good!

…

On Mar 20, 2018, at 20:04, Sriram ***@***.***> wrote: @digi0ps commented on this pull request. In src/options/settings/index.js: > @@ -0,0 +1,12 @@ +import React from 'react' + +import { Wrapper } from 'src/common-ui/components' +import SearchInjection from './components/SearchInjection' + +const Settings = () => ( + <Wrapper> Oooh. I thought wrapper was for the UI. Great second point. Will this be good: I do all the state related logic in Settings component and pass it to the SearchInjection component? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

poltak · 2018-03-20T14:33:09Z

Ah I think you just need to set "getTotalCount" to true as an arg when you call search() and it will return that (now only passes "query")

…

On Mar 20, 2018, at 20:15, Sriram ***@***.***> wrote: @digi0ps commented on this pull request. In src/search-injection/content_script.js: > + const component = document.getElementById('memexResults') + if (component) component.parentNode.removeChild(component) + + const target = document.createElement('div') + target.setAttribute('id', 'memexResults') + container.insertBefore(target, container.firstChild) + + // Number of results to limit + const limit = constants.LIMIT[position] + + // Render our React component on the target element + // Passing this same function so that it can change position + ReactDOM.render( + <Results + results={results.slice(0, limit)} + len={results.length} Ah, I was planning to look into this issue, as it never showed me a length greater than 10. But I don't think the there is a totalCount attribute in the payload received. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

digi0ps · 2018-03-22T11:40:41Z

@poltak @oliversauter @ShishKabab
Wrote few important tests. Modifed according to the comments. Cleaned up the code. And wrote a Readme.

Check it out and tell me if there anything else that needs to be done. Also, should I merge the commits?

ShishKabab · 2018-03-22T11:53:04Z

src/options/settings/components/container.js

+    }
+
+    async componentDidMount() {
+        const checked = await getLocalStorage(SEARCH_INJECTION_KEY, true)


Would name this to something more descriptive. Deeper into the code I see a checkbox is checked, but it would be nice if I could see what it represents immediately, like userDidEnableResults (or probably something better.)

blackforestboi · 2018-03-22T13:18:41Z

Notes to @poltak re the tracking:

We want to track active user stats (monthly, weekly, daily) when people actively use the results from the google integration. But not when they just search via google (and results are fetched)
We want to know when people disable the integration alltogether.

poltak · 2018-03-23T01:02:48Z

package.json

@@ -71,6 +71,7 @@
    "reselect": "^3.0.1",
    "response-to-data-url": "^0.1.0",
    "rxjs": "^5.5.5",
+    "sinon": "^4.4.6",


Pretty sure sinon should be a dev dep. (yarn remove sinon && yarn add --dev sinon)

poltak · 2018-03-23T02:03:24Z

src/options/settings/components/container.js

+import { getLocalStorage, setLocalStorage } from 'src/search-injection/utils'
+import { SEARCH_INJECTION_KEY } from 'src/search-injection/constants'
+
+class Container extends React.Component {


Calling the class just Container means you have to open it up and look inside to see what it does, as the name doesn't give much info. You also might want to add more containers in this dir later for whatever reason. Maybe SearchInjectionContainer would be better as it simply wraps around that view.

poltak · 2018-03-23T02:08:24Z

src/search-injection/content_script.js

+}
+
+const search = query => {
+    // query: (string) query to be search


For code doc, I would advise getting familiar with jsdoc which is what we're using in the rest of the project. Not super urgent - comments are better than nothing! - but just to keep in mind for future.

Oh okay, I will take a look at jsdoc.

poltak · 2018-03-23T02:29:25Z

@digi0ps there might have been a slight regression in one of the recent changes: content script throws an error now when it mounts the main React component as the props aren't being passed down anymore for some reason.

Also I would remove any console.logs from the content script code; it will log directly to the page's console rather than the extension's console.

digi0ps · 2018-03-27T16:07:23Z

@oliversauter @poltak Should I squash any of the commits?

Results are attached only after the DOM is loaded. elements weren't loaded.

Also: - Changed the way values are stored in localStorage. - Remove the need for background.js in search_injection

Also, - Removed fadeOut of RemovedTest. - Render React only when document has completed loading. - Change the logo used.

Also, link it in memex results.

Also, adds fade animation.

Also, cleans up content_script.js.

And, clean up background.js.

- trakcing when user disables - user flagged as active when they click a result or click show more to go to overview - all done from bg script via remote calls

- the settings gear icon had a weird grey background in FF that was sticking out

Also, ran yarn format to clean up code.

poltak · 2018-03-29T03:16:46Z

@oliversauter happy to merge this? (let me know and I can resolve the conflicts)

blackforestboi requested a review from ShishKabab February 24, 2018 13:33

digi0ps force-pushed the search-injection branch from c04b69e to 74badee Compare February 28, 2018 13:45

digi0ps force-pushed the search-injection branch 3 times, most recently from 8f022b3 to 9c37f2e Compare March 11, 2018 16:56

digi0ps force-pushed the search-injection branch 10 times, most recently from ccf113f to 237fa10 Compare March 18, 2018 19:53

digi0ps commented Mar 19, 2018

View reviewed changes

poltak requested changes Mar 20, 2018

View reviewed changes

digi0ps force-pushed the search-injection branch from bd16b60 to 0d2e46e Compare March 22, 2018 11:31

ShishKabab reviewed Mar 22, 2018

View reviewed changes

poltak reviewed Mar 23, 2018

View reviewed changes

digi0ps force-pushed the search-injection branch 2 times, most recently from d225e9d to 52c41cc Compare March 27, 2018 18:32

digi0ps and others added 22 commits March 28, 2018 13:50

Get search query from url and fetch Memex search results.

5e4cc29

Inject rendered Results component in Google results page.

29c2ae4

Results are attached only after the DOM is loaded. elements weren't loaded.

Enable show more results.

5daf949

Add toggle button in results page.

f954338

Enable css for content_scripts.

9bbc232

Refactor code and UI.

4c79f86

Also: - Changed the way values are stored in localStorage. - Remove the need for background.js in search_injection

Adds dropdown to settings.

079f3bd

Adds Remove Forever and a switch in options page.

96eb1c7

Enable users to change position of Memex results container.

28fece0

Tweaks to CSS.

0b02814

Also, - Removed fadeOut of RemovedTest. - Render React only when document has completed loading. - Change the logo used.

Add settings panel in options.

490237f

Also, link it in memex results.

Change links to options, inject above ads and css tweaks.

5c05b53

Also, adds fade animation.

Fixed CSS glitch.

1c3f40e

Clean up and write tests for utils.js

d51612b

Move DOM functions to a separate file.

d67b4d7

Also, cleans up content_script.js.

Add more tests to utils using sinon.

be639e5

Separate logic and view in React components.

53e7e81

And, clean up background.js.

Add Readme.

5a5d494

Set up tracking + active flag for search injection

c24d16f

- trakcing when user disables - user flagged as active when they click a result or click show more to go to overview - all done from bg script via remote calls

Quick fix for weird styles in FF

a602b19

- the settings gear icon had a weird grey background in FF that was sticking out

Changes to styles and text in settings.

0264cdd

Also, ran yarn format to clean up code.

Add yarn-error.log to .gitignore.

6d62a50

digi0ps force-pushed the search-injection branch from 52c41cc to 6d62a50 Compare March 28, 2018 08:27

poltak approved these changes Mar 29, 2018

View reviewed changes

Merge branch 'master' into search-injection

8b58f59

ShishKabab merged commit 2068880 into WorldBrain:master Mar 29, 2018

WIP: Search Injection #320

WIP: Search Injection #320

digi0ps commented Feb 24, 2018 •

edited

blackforestboi commented Feb 24, 2018 •

edited

digi0ps commented Feb 24, 2018

blackforestboi commented Feb 28, 2018

digi0ps commented Mar 11, 2018

blackforestboi commented Mar 11, 2018

digi0ps Mar 19, 2018

blackforestboi Mar 19, 2018

poltak left a comment

poltak Mar 20, 2018

poltak Mar 20, 2018

digi0ps Mar 20, 2018

poltak Mar 20, 2018

digi0ps Mar 20, 2018

poltak Mar 20, 2018

digi0ps Mar 20, 2018

poltak Mar 20, 2018

poltak Mar 20, 2018

poltak Mar 20, 2018

digi0ps Mar 20, 2018

poltak Mar 20, 2018

digi0ps Mar 20, 2018

poltak commented Mar 20, 2018 via email

poltak commented Mar 20, 2018 via email

digi0ps commented Mar 22, 2018

ShishKabab Mar 22, 2018

blackforestboi commented Mar 22, 2018

poltak Mar 23, 2018

poltak Mar 23, 2018

poltak Mar 23, 2018

digi0ps Mar 23, 2018

poltak commented Mar 23, 2018

digi0ps commented Mar 27, 2018

poltak commented Mar 29, 2018 •

edited

WIP: Search Injection #320

WIP: Search Injection #320

Conversation

digi0ps commented Feb 24, 2018 • edited

blackforestboi commented Feb 24, 2018 • edited

digi0ps commented Feb 24, 2018

blackforestboi commented Feb 28, 2018

digi0ps commented Mar 11, 2018

blackforestboi commented Mar 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poltak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poltak commented Mar 20, 2018 via email

poltak commented Mar 20, 2018 via email

digi0ps commented Mar 22, 2018

Choose a reason for hiding this comment

blackforestboi commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poltak commented Mar 23, 2018

digi0ps commented Mar 27, 2018

poltak commented Mar 29, 2018 • edited

digi0ps commented Feb 24, 2018 •

edited

blackforestboi commented Feb 24, 2018 •

edited

poltak commented Mar 29, 2018 •

edited