Skip to content

[WIP]Memex results alongside google search#323

Closed
subrat-sahu wants to merge 7 commits intoWorldBrain:masterfrom
subrat-sahu:search-injector
Closed

[WIP]Memex results alongside google search#323
subrat-sahu wants to merge 7 commits intoWorldBrain:masterfrom
subrat-sahu:search-injector

Conversation

@subrat-sahu
Copy link
Copy Markdown
Contributor

@subrat-sahu subrat-sahu commented Feb 28, 2018 •
edited
Loading

#281
webp net-gifmaker
Hide and Undo.

Done

  • Show alongside google search.
  • Toggle search injection.
  • Show more results on overview page.
  • Settings Dropdown
  • Maximize and minimize (remember settings)
  • Change Memex injection position(side and over)
  • Hide with Undo

Future goals:

  • Improve UI/UX

  • Refactor Code.

  • Add a more general Regex for google.

  • Apply for Bing, Duck Duck Go and more.

@subrat-sahu
Copy link
Copy Markdown
Contributor Author

subrat-sahu commented Feb 28, 2018

@oliversauter @poltak Please review the work done until now .

poltak
poltak reviewed Mar 1, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@poltak poltak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty amazing stuff @subrat-sahu ! Genuinely impressed at how quick you got this working and in a very elegant way! And pretty much no changes to existing code.

All the view stuff will change according to the updated designs, so ignored that stuff. But the rest of the code is pretty high quality; no big issues I can see.

Adding react to the content script is probably gonna boost up the size, but it's not really a major issue as this isn't a website - it's a web extension. Maybe a future enhancement could be write this UI in one of those super tiny UI libs.

May also be nice to add some JSDoc with type info to the main functions you export from your modules. Becomes a lot easier for future devs to use without looking at the code then

Awesome work so far!

Comment thread src/search-injector/background/index.js Outdated
}
})

async function openOverviewPagewithParams(queryParams) {
Copy link
Copy Markdown
Member

@poltak poltak Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a openOverview function in src/background module. Maybe you could integrate the queryParams param into that one and import here for re-use.

Comment thread src/search-injector/content_script.js
const fetchSearchInjection = remoteFunction('fetchSearchInjection')

// Render Memex search results to the Google Search Page
const getCmdMessageHandler = ({ cmd, ...payload }) => {
Copy link
Copy Markdown
Member

@poltak poltak Mar 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will all be much simpler once we get issue #288 finished :) then just call a function rather than have the port setup.

Comment thread src/search-injector/components/App.js Outdated
}
}

handleShowMoreResults = () => {
Copy link
Copy Markdown
Member

@poltak poltak Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verb handle usually reserved for methods/prop fns that will be used as event listeners. Any method that returns JSX generally begins with the verb render, hence render methods.

Comment thread src/search-injector/background/interface.js Outdated
SEARCH_INJECTION_KEY,
)
// For old browsers do typeof searchInjection === "undefined"
if (searchInjection === undefined) {
Copy link
Copy Markdown
Member

@poltak poltak Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can supply a default value like browser.storage.local.get({ [SEARCH_INJECTION_KEY]: true }) then you just need to return that value. Shouldn't be a need to call storeSearchInjection if it's not defined yet.

Copy link
Copy Markdown
Contributor Author

@subrat-sahu subrat-sahu Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poltak I need to make the store the function more generalised so i can use it to store more variables so I will change it any ways

Comment thread src/search-injector/background/interface.js Outdated
export async function toggleSearchInjection() {
const searchInjection = await fetchSearchInjection()
const value = searchInjection === false
return await storeSearchInjection(value)
Copy link
Copy Markdown
Member

@poltak poltak Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More conventional way to toggle is just go storeSearchInjection(!searchInjection). ! operator will do the inversion for your with an explicit comparison needed

Copy link
Copy Markdown
Contributor Author

@subrat-sahu subrat-sahu Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poltak GREAT !

Comment thread src/search-injector/background/index.js Outdated
// Temporary placement of this functions
browser.runtime.onMessage.addListener(value => {
switch (value.action) {
case 'openOverviewPagewithParams':
Copy link
Copy Markdown
Member

@poltak poltak Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well define this in the constants module; used in multiple places

Comment thread src/options/searchInjection/index.jsx Outdated

render() {
return (
<Wrapper>
Copy link
Copy Markdown
Member

@poltak poltak Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for <Wrapper> here. This component renders no DOM, it's just used to allow placement of sibling JSX elements without an explicitly defined parent.

Comment thread src/options/searchInjection/index.jsx
}

async handleToggle(event) {
await toggleSearchInjection()
Copy link
Copy Markdown
Member

@poltak poltak Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just have toggleSearchInjection return the updated value? Not super important, but saves a remote call

Copy link
Copy Markdown
Contributor Author

@subrat-sahu subrat-sahu Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poltak Ok I will see into it !

@subrat-sahu
Copy link
Copy Markdown
Contributor Author

subrat-sahu commented Mar 9, 2018

@oliversauter can you please check, that are the requirements fulfilled !

@blackforestboi
Copy link
Copy Markdown
Member

blackforestboi commented Mar 9, 2018

Good stuff, works except for a couple of things :)

A few things we still need to improve:

  • sometimes it does not load the search box right from the beginning, or relatively late. I think @digi0ps solved this. Can you look at the code and give @subrat-sahu some hints?
  • "show all results" does not work
  • in "above" version, only show 3 results
  • In terms of styling, orient yourself at the spacing/font-size/font-family of the google results. The only differentiator should be the grey box around Memex Results. screen shot 2018-03-09 at 10 56 13
  • Move the memex icon so it is horizontally aligned with the right border of the box
  • Settings icon in "above" version looks vertically compressed and not square.
  • Good idea to change the color of the onhover box, I'd remove the underline onhover though.
  • When 'minimizing', the width and location of all elements should stay the same.
    Now, the overall width gets smaller, and the 'show all results' moves around. (see: http://recordit.co/6YwYf4rBz2)
  • When results are on top of google results, everything seems to be a bit too far right. Width and position should align with Google Results. screen shot 2018-03-09 at 11 08 16
  • On the bottom of this comment are mockups for some designs provided by Jean-Baptiste. He will bring an update soon, but they should be good for now. Things I'd change though:
    • Date should be vertically aligned with the url, so that the title can span the full width unless it overflows.
    • Box of "above results" should be no wider than the google results
    • Fontstyle like google results
  • Color of "Found...in your" should be #77777 or a bit darker (seen in 'above results' screenshot) and number bold with #3EB995

Mockups above and next to results:

group 10
group 11

@subrat-sahu
Copy link
Copy Markdown
Contributor Author

subrat-sahu commented Mar 9, 2018

@oliversauter what about the Hide/Undo ? is it ok? I have the UI in my todo list I will right away start working on the changes. :)
Thanku !

@blackforestboi blackforestboi mentioned this pull request Mar 11, 2018
Merged
10 tasks
@subrat-sahu
Copy link
Copy Markdown
Contributor Author

subrat-sahu commented Mar 19, 2018 •
edited
Loading

@oliversauter I thinkI fixed all the major issues here, only CSS changes are to be done. Please do take a look

  • Fixed Show more results

  • Results load faster

  • improved css still work need to be done

  • All features working

    • change position
    • show all results
    • minimize/maximize(remember settings)
    • Hide with undo that disappears after 10 secs.

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

Reviewers

@poltak poltak poltak left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@subrat-sahu @blackforestboi @poltak