Skip to content
This repository has been archived by the owner on Jan 26, 2023. It is now read-only.

Browser Extension Beta #5

Merged
merged 22 commits into from Dec 19, 2017
Merged

Browser Extension Beta #5

merged 22 commits into from Dec 19, 2017

Conversation

jclancy93
Copy link
Contributor

@jclancy93 jclancy93 commented Nov 23, 2017

WIP for gitcoinco/web#50

TODO List

Development

  • 1. Ability to browse funded issues (and amounts) on an issue board
  • 2. View funded issue states (and amounts) on github issues page
  • 3. Visual style that matches github
  • 4. ability to input what keywords you want to filter on, and only display those issues in the dropdown.
  • 5. The notifications are kind of obnoxious. Don't show notifications for all issues associated with repo.
  • 6. More robust check for user page
  • 7. Improve user tip button (to match github styles)

@jclancy93
Copy link
Contributor Author

jclancy93 commented Nov 24, 2017

Here is the current state of a repos issues page with badges for issues with funding amounts and statuses (these are also links which take you to the issue on the gitcoin site)

image

image

Here is the current state of a search for keywords, I am doing a keyword match as well as doing a title match with indexOf() . One can also search for claimed bounties. It is currently working on a 500ms debounce from the 'keyup event'.

image

image

Here is the new user tip button.

image

@owocki let me know if you have any thoughts on the work so far.

@owocki
Copy link
Contributor

owocki commented Nov 24, 2017

i am out of pocket today (limited internet, thanksgiving break in the US), but wanted to pop in and say that i'm very excited by the progress i see here! the issues board in particular looks very promising.

@owocki
Copy link
Contributor

owocki commented Nov 24, 2017

i will pop in with more thoughtful feedback hopefully over the weekend

@jclancy93
Copy link
Contributor Author

No rush @owocki, Happy Thanksgiving! I will update this pull with any further progress I make

@jclancy93
Copy link
Contributor Author

jclancy93 commented Nov 24, 2017

I actually have a few things I want to clarify:

For "1.) Ability to browse funded issues (and amounts) on an issue board", when you say board, do you mean the "boards" tab (the kanban board) or the page i attached (all issues page)? In your previous comment you said my work on the issues board looks good, just want to make sure which page this task is referring to.

I am also struggling to find the code when the notifications are generated, could you point me in the right direction? Thanks you for the help and I'm glad you like the progress so far :)

@owocki
Copy link
Contributor

owocki commented Nov 27, 2017

Looking at this now. Thanks for your patience thus far (holiday weekend!)

@@ -11,25 +11,41 @@ var body = function(){
setThumbnail('');
}

var isOnUserProfile = isOnGitHub && url.match(/.+\/.+\/?/gi) != null;
var isOnUserProfile = isOnGitHub && url.match(/.+\/.+\/?/gi) != null && document.getElementById("report-block-modal");
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch!

var isOnProfile = isOnGitHub && url.match(/.+\/.+\/.+\/?/gi) != null;
var isOnRepo = isOnGitHub && url.match(/.+\/.+\/.+\/.+\/?/gi) != null;
var isOnIssuePage = isOnGitHub && ( url.indexOf('/pull/') != -1 || url.indexOf('/issue/') != -1 || url.indexOf('/issues/') != -1 );
var isOnIssuesPage = isOnGitHub && url.indexOf('/issues') != -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

this reminds me, i should refactor isOnIssuePage to be more descriptive of where the user actually is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

definitely, this is something I can add too. A simple "check if element on page" should suffice, unless you have something else in mind

}

getAllBounties = function(){
var bounties_api_url = 'https://gitcoin.co/api/v0.1/bounties/?order_by=-web3_created';
Copy link
Contributor

Choose a reason for hiding this comment

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

if you wanted to be more performant/selective, you could search bounties by keyword here (so that you don't have to match inside the for loop on line 28).

Here's an example: https://gitcoin.co/api/v0.1/bounties/?raw_data=python

Copy link
Contributor

Choose a reason for hiding this comment

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

itd also be good to only show mainnet bounties that are open

https://gitcoin.co/api/v0.1/bounties/?&idx_status=open&network=mainnet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, thanks for this

Copy link
Contributor

Choose a reason for hiding this comment

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

mind making this change

itd also be good to only show mainnet bounties that are open

https://gitcoin.co/api/v0.1/bounties/?&idx_status=open&network=mainnet

@@ -66,15 +75,69 @@ var addMessage = function(_class, msg, seconds=5000){
setTimeout(callback, seconds);
}

getAllBounties = function(){
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if theres a way to DRY here.

@owocki
Copy link
Contributor

owocki commented Nov 27, 2017

installing the browser extension now to play with it. here is my feedback upon first use:

  • could the keywords persist from poup to popup? it stinks to lose my place just because i closed the browser extension window.
  • better yet, the keyword could prefill with the repo name of whatever repo the user is browsing at the moment (but only if they are on github).
  • building on the above, if the setThumbnail() number always matched the # of bounties that were visible when you opened the popup, that'd be 💯
  • issues page looks great!
  • so does the new tip button!
  • there used to be a 'fund issue' or 'claim issue' button on issue detail pages (example: Provide Firefox version of Gitcoin Chrome Extension #1) depending upon the gitcoin issue state. but maybe it's broken now.. ? Thats what I meant to refer to when I said "View funded issue states (and amounts) on github issues page". I should have said "issue detail page".

@owocki
Copy link
Contributor

owocki commented Nov 27, 2017

For "1.) Ability to browse funded issues (and amounts) on an issue board", when you say board, do you mean the "boards" tab (the kanban board) or the page i attached (all issues page)? In your previous comment you said my work on the issues board looks good, just want to make sure which page this task is referring to.

I was hoping that the user could be provided an option to hide non-funded issues when browsing an issues board. This way they would only be looking at funded issues. Is this plausible?

I am also struggling to find the code when the notifications are generated, could you point me in the right direction? Thanks you for the help and I'm glad you like the progress so far :)

Sure thing. What I did might be a little hacky, but here it is.

https://github.com/gitcoinco/chrome_ext/blob/master/script/pageload/callbacks.js#L25

You want to use setThumbnail()

@owocki
Copy link
Contributor

owocki commented Nov 27, 2017

Visual style that matches github

I'd also say that so far this looks really good and matches Github style way better than my code did.

@owocki
Copy link
Contributor

owocki commented Nov 27, 2017

TODO:

if an issue is already funded the green button shouldnt say 'fund issue', it should say 'claim issue'
also, if there is a long title, we need to deal with the CSS!

http://bits.owocki.com/1P321E1Z2Q2A/Image%202017-11-27%20at%209.13.27%20AM.png
MetaMask/metamask-extension#2577

@jclancy93
Copy link
Contributor Author

@owocki Thanks! Glad you like it. I tried to reuse github style classes as much as possible to save me writing css :)

I noticed the long titles conflicting with the button too. I will reposition so that doesn't happen and change text to claim issue if issue is funded.

I don't know how much success we will have with the issues kanban board. I can inject the html correctly on initial load, but it seems that there is a virtual scroll that re-renders the html and removes whatever i injected. Just pushed that code now, if you wanna take a look for yourself.

All your requested changed look good, I will work on them on the next few days and will update you with my progress.

@jclancy93
Copy link
Contributor Author

Also, as a side issue, I have noticed that the body function is only invoked on a full page refresh. From what I can tell, github is a collection of smaller SPAs. This means that not every page change will re-invoke the body function (it won't work if you stay inside the same SPA, only if you change to diff SPA). I tested and found this behavior on the master branch too.

Just something I thought I should give you a heads up on @owocki

@owocki
Copy link
Contributor

owocki commented Nov 27, 2017

I don't know how much success we will have with the issues kanban board. I can inject the html correctly on initial load, but it seems that there is a virtual scroll that re-renders the html and removes whatever i injected. Just pushed that code now, if you wanna take a look for yourself.

totally hacky, but would it be worth having a setInterval that keeps checking for re-rendered HTML and then inserts the gitcoin HTML?

Also, as a side issue, I have noticed that the body function is only invoked on a full page refresh. From what I can tell, github is a collection of smaller SPAs. This means that not every page change will re-invoke the body function (it won't work if you stay inside the same SPA, only if you change to diff SPA). I tested and found this behavior on the master branch too.

sorry, which body function? mind pointing me to the code?

i wonder if this means the setInterval() route might be the way to go more generally. its the only way i can think of handling this unless we can find a way to inject gitcoin code into github

@owocki
Copy link
Contributor

owocki commented Nov 27, 2017

All your requested changed look good, I will work on them on the next few days and will update you with my progress.

🔥 hott! looking forward

@jclancy93
Copy link
Contributor Author

totally hacky, but would it be worth having a setInterval that keeps checking for re-rendered HTML and then inserts the gitcoin HTML?

I tried doing something like this (was listening to dom changes) but it was firing way too much, setting it on an interval is a good idea. It won't be perfect but it will work.

sorry, which body function? mind pointing me to the code?

i wonder if this means the setInterval() route might be the way to go more generally. its the only
way i can think of handling this unless we can find a way to inject gitcoin code into github

Its the body function in the scripts/pageload/body.js file, invoked in body.js:52. I thought about how to handle this situation for a while over the weekend (with little success). I could see a setInterval being problematic for the general case. Wouldn't it just re-inject the same html multiple times? We could do a check to see if the html had already been injected as a workaround to that issue. It would be great if we could listen for a url change event instead. Ill let you posted on this.

@rajatkapoor
Copy link

Usually while developing chrome extension injecting content scripts to a dynamically changing SPA, we use MutationObservers. That might help in this case. Instead of calling the body() method in the content script on page load, we can have mutation observers observing added nodes and acting when an "interesting" change happens

@owocki
Copy link
Contributor

owocki commented Nov 28, 2017

this is so cool. thanks @rajatkapoor !

@jclancy93
Copy link
Contributor Author

jclancy93 commented Nov 29, 2017

@rajatkapoor thanks for pointing this out. I tried using the mutation events api with little success. mutation observers are exactly what I'm looking for.

@jclancy93
Copy link
Contributor Author

Here's a few more updates:

Fixed issue detail page positioning and text content
image

Early version of issue board page
image

The board page is tough to do. I tried hiding non-gitcoin issues but because the way these boards are designed, the element doesn't move up to the top of the list, its keeps its current position, because every card is positioned absolutely. This results in a bunch of whitespace when you try and hide cards.
image

Will post more updates soon. LMK if you have any thoughts.

@owocki
Copy link
Contributor

owocki commented Dec 7, 2017

really excited about the trello bonus! great to have you back @jclancy93 .. i hope to ship this soon!

@jclancy93
Copy link
Contributor Author

Hey @owocki, thanks for bearing with me! I added persistent keywords to the popup and the popup will now autofill the issue keyword when on a github repo. From what I can tell, it looks like you use the bounty smart contract as the data source for numBounties, the number for setThumbnail(). Would you like me to change this as the data source?

@owocki
Copy link
Contributor

owocki commented Dec 13, 2017

@jclancy93 npnp :)

Would you like me to change this as the data source?

defer to you on this. you can use either the REST API or web3 !

@jclancy93
Copy link
Contributor Author

@owocki it looks like the variable numBounties in the contract isn't always matching up with the number we expect (most of the time it is). For example, the smart contract repo it lists the number of issues as 4 but It should only be one.

@owocki
Copy link
Contributor

owocki commented Dec 13, 2017

got it; i think we can use the http api as source of truth then !

@jclancy93
Copy link
Contributor Author

jclancy93 commented Dec 14, 2017

Ok this should help a lot with the thumbnail issue. The thumbnail should now:

  1. When user on a github repo with gitcoin issues, display number of gitcoin issues in thumbnail
  2. When user on a github repo without gitcoin issues, display 0 in thumbnail
  3. When user not on a github repo, don't display a thumbnail

Let me know what you think!

@owocki
Copy link
Contributor

owocki commented Dec 14, 2017

Let me know what you think!

this sounds right to me. ill pull the lates code into my google chrome and test it for a day or two and get back if anything feels off

@owocki
Copy link
Contributor

owocki commented Dec 14, 2017

When user on a github repo

Is it only when you are on the proper repo, or anything downstream from the repo (wiki, pull requests, etc)?

display number of gitcoin issues in thumbnail

does it only do issues of a certain type?

for example, i am on this URL right now #5 (comment) and i get a 0 in my thumbnail -- but the issue we are on now (chrome extension) is an open issue i think..

@owocki
Copy link
Contributor

owocki commented Dec 14, 2017

one other random piece of feedback. it'd be cool if when i was on a github repo that has open funded issues, if those were automatically shown when i click the gitcoin thumbnail.

so for example, while on #5 i click the thumbnail and the keywords field is prefilled with gitcoinco/chrome_ext, thereby filtering the popup on those issues!

@jclancy93
Copy link
Contributor Author

jclancy93 commented Dec 14, 2017

Is it only when you are on the proper repo, or anything downstream from the repo (wiki, pull requests, etc)?

I know its working on both issues and the repo page, but I still need to do more testing downstream. Based on some quick testing I did just now, it doesn't seem to work on anything more downstream than issues, but I will fix that up.

for example, i am on this URL right now #5 (comment) and i get a 0 in my thumbnail -- but the issue we are on now (chrome extension) is an open issue i think..

So the Browser Extension Revamp issue is actually associated with the web repo, thats why its not showing up here. I am only displaying issues with idx_status=open right now, but can easily change that.

one other random piece of feedback. it'd be cool if when i was on a github repo that has open funded issues, if those were automatically shown when i click the gitcoin thumbnail.

This is already happening! :) Here's some screenshots:

image

image

@owocki
Copy link
Contributor

owocki commented Dec 18, 2017

awesome!

what can i do to help you get this shipped soon. this is already such an advancement over the current featureset, i'd love to put it live for the community to play with it soon :)

@jclancy93
Copy link
Contributor Author

Maybe we could release this on a beta branch and allow for the community to given open feedback/suggestions? You can announce it via slack or email or reddit?

I'd like to do a little cleanup before then, just remove logging statements and make sure that I'm consistent with your code. Apart from that, I think everything from the original bounty is complete.

The only other thing I think needs improving is running specific functions with setTimeouts, I'm hesitant to force everything to run on a setTimeout cause I'm afraid of the performance costs.

I'll try to have a push out later today with these two improvements. Let me know what you think of this plan

@owocki
Copy link
Contributor

owocki commented Dec 18, 2017

Maybe we could release this on a beta branch and allow for the community to given open feedback/suggestions? You can announce it via slack or email or reddit?

Yes, I like this idea.

I'd like to do a little cleanup before then, just remove logging statements and make sure that I'm consistent with your code. Apart from that, I think everything from the original bounty is complete.

The only other thing I think needs improving is running specific functions with setTimeouts, I'm hesitant to force everything to run on a setTimeout cause I'm afraid of the performance costs.

I'll try to have a push out later today with these two improvements. Let me know what you think of this plan

I'm into it :) While youre at it, can you resolve the merge conflict too pls?

@owocki owocki merged commit 23a91eb into gitcoinco:master Dec 19, 2017
@owocki
Copy link
Contributor

owocki commented Dec 19, 2017

merging to master so i can cut a release. stay tuned!

@owocki owocki changed the title (WIP) Browser Extension Revamp Browser Extension Beta Dec 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants