Contributors

[sophyphreak]

Hey, I added a simple list of all the current contributors. With this set up, you add the linkedin links manually, I know this feature isn't finished. You said you were thinking of a way to connect this up to the database?

[monarchwadia]

Hi @sophyphreak

This is great! Thank you!!!

I have a few suggestions for changes. Do you think we can do the following?

Change Request

Description of Change

This is where this pull request is right now:

[Frontend]-----------------> [github.com]

This is where I'd like the pull request to do before we merge it in:

[Frontend] --> [Backend] --> [github.com]

Suggested Path

Can you please:

• [ ] BACKEND: Create a GET /api/v1/contributors endpoint that hits Github
• [ ] FRONTEND: Instead of hitting Github, hit this endpoint to fetch the data

Why this is a good pattern

This is a good pattern for us because:

1. It decouples the frontend from a direct dependency on Github
2. It allows us to eventually implement caching for this data on the backend
3. It allows our database to have foreign-key references to that cached data
4. It makes the code a lot more consistent, maintainable, and flexible

EDIT: Also, seems like there are conflicts. Could you please resolve them?

[monarchwadia]

@sophyphreak I'm not sure how familiar you are with the backend -- please let me know if I can help you understand more :-)

[sophyphreak]

Can do. I'll let you know if I have any trouble. Also, I'm surprised about the merge conflict. I should be able to fix that. Thanks!

[sophyphreak]

Github seems to believe there are no merge conflicts, so I think you fixed that problem when you merged my branch with the master branch.

I added a route to the backend to get the contributors info from the Github api. Everything seems to work, but the styling in the contributors component is different due to a change in the CSS. It functions, however, so I decided to leave it how it is.

Is there anything more I can work on with this component, or should I move onto something else in the project? I'm happy to help however I can.

[sophyphreak]

@monarchwadia, any problems?

[monarchwadia]

Hi @sophyphreak , checking this now!

[sophyphreak]

In the process of making requested changes.

[sophyphreak]

I'm getting an error from the github server.

{

    "message": "API rate limit exceeded for 73.155.178.206. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
    "documentation_url": "https://developer.github.com/v3/#rate-limiting"

}

The docs say we have a limit of 5000 requests per hour. I assume someone working on this went over that?

[monarchwadia]

I'm getting an error from the github server.

{

    "message": "API rate limit exceeded for 73.155.178.206. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
    "documentation_url": "https://developer.github.com/v3/#rate-limiting"

}

The docs say we have a limit of 5000 requests per hour. I assume someone working on this went over that?

@sophyphreak I think that refers to your own local IP address (which could include multiple users in addition to you)

This is why this needs to be on the backend -- and also throttled + cached. I can add a ticket to build the cache after this ticket is over.

[sophyphreak]

Okay. Makes sense. Thanks!

[sophyphreak]

Hey @monarchwadia, I believe I've finished all the requested changes, but let me know if this needs more work before it can be merged into master. Thanks for the thorough code review and specific directions. It made my part a lot easier.

[carloshdelreal]

The problem that I saw with the table of contributors is in small devices

[sophyphreak]

@carloshdelreal, thank you for your feedback. Unfortunately, I'm having some trouble understanding some of your suggestions, and I am requesting clarification.

You said,

I have to say that there is a lot to do in the frontend for readability, could you try to do it better??

I'm sorry to say this is a very nonspecific request. Can you be more specific?

the blue table border should enclose each of the contributors data to highlight them and to give a sense of separate entities. As it is, just encloses all the contributors on a single blue box and it's very difficult to identify different elements.

The blue border is inherited from global CSS values, so I assumed it's continuing some theme. Can you be more specific about how you believe I can improve readability?

[sophyphreak]

@carloshdelreal

The problem that I saw with the table of contributors is in small devices

I was following the advice from this article for building mobile responsive tables. Do you have another suggestion for building tables that will work?

https://css-tricks.com/responsive-data-tables/

[sophyphreak]

And @monarchwadia, if you can restate and/or clarify any of @carloshdelreal's suggestions, I would definitely appreciate it. Thanks!

[carloshdelreal]

@carloshdelreal

The problem that I saw with the table of contributors is in small devices

I was following the advice from this article for building mobile responsive tables. Do you have another suggestion for building tables that will work?

https://css-tricks.com/responsive-data-tables/

Personally I don't like to use tables, I definitely preffer to use Bootstrap to arrange the items in a single row, I think is easier to have control on the sizes of the elements as well as the row sizes

[sophyphreak]

@carloshdelreal, makes sense, but I believe @monarchwadia specifically requested a table.

[carloshdelreal]

I promise I will try to be more specific in the reviews, check this out

• Remember to run yarn lint to fix linting issues
• I am having an unknow element error

[carloshdelreal]

@carloshdelreal, makes sense, but I believe @monarchwadia specifically requested a table.

so I will blame @monarchwadia

[sophyphreak]

I promise I will try to be more specific in the reviews, check this out

* Remember to run `yarn lint` to fix linting issues

* I am having an unknow element error

Thank you for being more specific in reviews.

I ran yarn lint on the files I changed, but it looks like a lot of other files need to be fixed as well. To keep this pull request limited to the specific feature, I'm only updating the files I was already updating.

The console error was because I put a partial inside the template. I've updated the code so that it doesn't use a partial, so the console error no longer appears.

[sophyphreak]

Okay, @carloshdelreal fixed the addHttpPrefix function. I think it's ready to merge to master. What do you think?

[carloshdelreal]

@monarchwadia Though, I would like to know how to cache the github contributing api data so that our backend doesn't have to request the data to the github api every time a user sees the contributors page and therefore potentially been banned because too many requests.

[sophyphreak]

@monarchwadia, I believe this pull request is ready to be merged. 😊

[sophyphreak]

@monarchwadia, any problems?

[sophyphreak]

@monarchwadia, I'm sure you're really busy, but I just wanted to make sure this pull request doesn't go fallow.

[monarchwadia]

Hi @sophyphreak @carloshdelreal thanks for the followups! I was going to announce in today's townhall meeting that PRs will all be merged in and deployed once authentication is done (it will be done by Sunday)

Sorry for being incommunicado, I've been very heads-down with several things, but I assure you this is on top of mind for me.