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

Move to GitHub OAuth from bare GitHub username entry #4

Open
gaganmalvi opened this issue Oct 1, 2021 · 13 comments · Fixed by #76
Open

Move to GitHub OAuth from bare GitHub username entry #4

gaganmalvi opened this issue Oct 1, 2021 · 13 comments · Fixed by #76
Labels
enhancement New feature or request hacktoberfest help wanted Extra attention is needed

Comments

@gaganmalvi
Copy link
Member

gaganmalvi commented Oct 1, 2021

  • As of now, we only enter the GitHub username to ping the GitHub API and that sends back a basic JSON response that we parse to provide a small and simple profile.
  • But for different usecases like viewing repositories and the names of followers GitHub only sends back the first 30 results, which is not useful.
  • Authenticate via GitHub OAuth since that is required for production apps.
@gaganmalvi gaganmalvi added enhancement New feature or request hacktoberfest labels Oct 1, 2021
@rishuyadav rishuyadav added the help wanted Extra attention is needed label Oct 2, 2021
@V9vek
Copy link
Contributor

V9vek commented Oct 5, 2021

Hey @gaganmalvi 👋🏼
I was looking into this issue and I understood that GitHub API sends only 30 responses at a time

I checked the GitHub API Docs and found that while fetching repositories data we can set per_page as a query parameter in the URL, which can be set upto 100 results at a time, so we will be getting 100 repos or followers at a time

@V9vek
Copy link
Contributor

V9vek commented Oct 5, 2021

But you said to integrate GitHub OAuth for this issue? Is there any particular reason to use oauth?

I guess we can use Paging to fetch data in chunks/pages to solve this issue

Any thoughts or corrections?

@rishuyadav
Copy link
Member

rishuyadav commented Oct 5, 2021

Hey, @V9vek the reason for the auth was we were also planning some post stuff but for now, we will stick to the basic read-only app so yeah u can go ahead with that per_page thing so that users can fetch all repos on screen.
If you are interested in taking up this issue, we can assign you

@V9vek
Copy link
Contributor

V9vek commented Oct 5, 2021

@rishuyadav you got me somewhere wrong, as I was saying as of now we can fetch 30 repositories at a time, but with per_page as a query parameter in the URL, which can be set up to 100 results at a time
I mean now we can fetch 100 repositories BUT, if someone has more than 100 repositories, those can't be shown

We still can't fetch all repos on screen, but first 100 repos only

Do let me know if you want that upgrade, I will open a pull request so you can check and review it

@rishuyadav
Copy link
Member

rishuyadav commented Oct 5, 2021

@rishuyadav you got me somewhere wrong, as I was saying as of now we can fetch 30 repositories at a time, but with per_page as a query parameter in the URL, which can be set up to 100 results at a time I mean now we can fetch 100 repositories BUT, if someone has more than 100 repositories, those can't be shown

We still can't fetch all repos on screen, but first 100 repos only

Do let me know if you want that upgrade, I will open a pull request so you can check and review it

Yes i got ur point I went through the API documentation also, for now we can move ahead with 100, no probs

@rishuyadav
Copy link
Member

Hey @V9vek are you working on this issue, we have to fetch 100(max allowed) for each type of data being displayed in app 👍

@V9vek
Copy link
Contributor

V9vek commented Oct 6, 2021

I can write the code for fetching the first 100, but NOT Pagination in which we fetch data in pages/chunks
I mean as of now we are seeing only 30 repositories/followers(some data), but afterward we will see 100, but never beyond 100

If you guys want that, I can open a PR regarding the same, do let me know

@rishuyadav
Copy link
Member

Yep sure go ahead, and please do it for the Followers & Following section also 👍

@V9vek
Copy link
Contributor

V9vek commented Oct 6, 2021

Opened a PR #60
Do check it out and run on your devices too
But I think Pagination is a better solution for this issue
Using Pagination, we can fetch 100 results per page and further beyond 100 in chunks/pages and user can see all its repo/followers/following

Keep this issue open, maybe someone would apply Pagination to it
Thanks 🙏🏼 🚀

@rishuyadav
Copy link
Member

Merged the pr, thanks for contributing

@garvsgit
Copy link
Contributor

Do we still need oauth or are we done now? If oauth is to be added then what scope is needed for the future plans of gitpositive(currently it is read only)?

@Devendra34
Copy link
Contributor

Hi, @rishuyadav, I can contribute to implement pagination in the app.
Can you please assign a me this/new issue for the same?

@Devendra34
Copy link
Contributor

Thanks @rishuyadav for assigning it to me.
I found that pagination was already implemented but not in a standard way.
I have opened #76 by updating the implementation using Jetpack's paging library and also fixed the progress bar issue(I found that progess bar was not shown before initial load).

@rishuyadav rishuyadav linked a pull request Oct 27, 2021 that will close this issue
@Devendra34 Devendra34 removed their assignment Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hacktoberfest help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants