Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Firefox #45

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@KennethAshley
Copy link

commented Feb 17, 2018

Why?

We want to be able to add the gitcoin extension to firefox.

What Changed?

  • Used Web Extensions API to make calls universal.
  • Updated jQuery
  • Fixed innerHTML issue
References

#1

@owocki

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

not sure why, but this version of the extension seems to not recognize the existence of metamask => http://bits.owocki.com/3D3v3v3d0v29/Screen%20Shot%202018-02-26%20at%204.52.33%20PM.png


```
npm install --global web-ext
web-ext build

This comment has been minimized.

Copy link
@owocki

owocki Feb 26, 2018

Member

running these commands seems to generate a folder called web-ext-artifacts/... should this folder be added to .gitignore?

This comment has been minimized.

Copy link
@KennethAshley

KennethAshley Feb 28, 2018

Author

Good idea. I think we should add it actually

@owocki

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

do i need to worry about these warning messages when loading the temporary add on to firefox?

Reading manifest: Error processing background.persistent: Event pages are not currently supported. This will run as a persistent background page.
Reading manifest: Error processing storage: An unexpected property was found in the WebExtension manifest.
@owocki

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

not sure why, but this version of the extension seems to not recognize the existence of metamask => http://bits.owocki.com/3D3v3v3d0v29/Screen%20Shot%202018-02-26%20at%204.52.33%20PM.png

this seems to happen in both chrome / firefox

@gasolin
Copy link
Collaborator

left a comment

Sorry I found several necessary changes are still missed...
You can refer the old branch which already deals with several enhancement (but without test at that time) https://github.com/gitcoinco/chrome_ext/tree/master_needs_regression_test

For example:

  1. upgrade to jquery 3 doesn't just upgrade the lib itself, you also need to upgrade the related api (was fixed in 3fdd1a7)
  2. jquery append is not allowed when last time we got the addon review (was fixed in be2ea38)
  3. build firefox command & related .gitignore configuration (was fixed in abb6c42)
@KennethAshley

This comment has been minimized.

Copy link
Author

commented Feb 28, 2018

@gasolin @owocki So issue this issue closed? I saw all the work gasolin did. Great job!

@owocki

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

no i dont think so.. id still like to get this to the finish line

@gasolin

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2018

@KennethAshley no some API is not fully migrated yet and the previous branch is not test covered. You can take the bounty by cherry-picking those PR and merge to the master(I should do that but currently occupied), and fix the rest of issues.

@owocki

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

for context, i reverted a lot of that code in the branch because of regressions. here are my notes from when i did so #44

@@ -156,16 +156,6 @@ $(document).ready(function(){
$("#username").text("@" + localStorage['githubusername'] );
}

if(!isweb3()){

This comment has been minimized.

Copy link
@owocki

owocki Mar 8, 2018

Member

itd be great if this still worked in chrome.. i just think we should remove it from firefox so we can cut a release there.

@KennethAshley

This comment has been minimized.

Copy link
Author

commented Mar 14, 2018

@owocki I actually think the use of the older version of jQuery is blocking us.

image

@owocki

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

shoot..

maybe we could just reference the jquery veresion from the CDN instead of trying to build / package it internally? or maybe we need to update to the latest jqeury

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.