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

Refactor the codebase #213

Merged
merged 2 commits into from
Jun 3, 2019
Merged

Refactor the codebase #213

merged 2 commits into from
Jun 3, 2019

Conversation

Rishabh570
Copy link
Collaborator

Fixes #201
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • PR body includes fixes #0000-style reference to original issue #
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

@Rishabh570
Copy link
Collaborator Author

@jywarren @sagarpreet-chadha @harshithpabbati I've opened this PR for #201, please take a look.

Sorry, I've made a single commit for all of it, I can break it down into smaller commits if you want...

Here is my gh-pages link for current changes: http://rishabh570.me/community-toolbox/

@jywarren
Copy link
Member

Oh wow! Huge!

So, this is awesome. I did get some kind of error here, though - can you take a look?

image

Great work!!!

@Rishabh570
Copy link
Collaborator Author

@jywarren Hmm, I'm not facing this error, I guess hard refreshing the page would resolve this error...I'll take a look.
Thanks 😃

@Rishabh570
Copy link
Collaborator Author

or maybe disabling the browser cache from network tab...

@Rishabh570
Copy link
Collaborator Author

Hi @jywarren, I've made a GIF of how these changes work in my local version of the site...I don't know why gh-pages is not reflecting the latest changes. Please take a look,

REFACTOR_GIF

@Rishabh570
Copy link
Collaborator Author

Pinging @jywarren @publiclab/reviewers, please take a look :)

@rexagod
Copy link
Member

rexagod commented Jun 1, 2019

@Rishabh570 Can you verify if your promises are getting resolved properly? Skimming through the errors this seems to be an async issue.

@Rishabh570
Copy link
Collaborator Author

@rexagod yeah so the errors showing up on @jywarren 's screenshot were related to database setting up...actually if we try to do any kind of db operation while db is setting up (it takes some time to set-up, some milliseconds) that error gets showed up. But that's not from my current changes as I've shared a screenshot above that doesn't contain those errors...

@Rishabh570
Copy link
Collaborator Author

But don't know why gh-pages are not reflecting that...

@rexagod
Copy link
Member

rexagod commented Jun 1, 2019

actually if we try to do any kind of db operation while db is setting up (it takes some time to set-up, some milliseconds) that error gets showed up

Did you set up some sort of async wrapper to handle this? If no, consider wrapping this in a async/await function, or maybe return a promise even. But if you have done something like this, I guess you should increase the timeouts (not recommended).

Also, is this live? I can provide more input on taking a closer look.

@Rishabh570
Copy link
Collaborator Author

@rexagod Yes, so it returns a promise ( there are promises for every async process ).

Yeah, my changes are live at http://rishabh570.me/community-toolbox/

@rexagod
Copy link
Member

rexagod commented Jun 1, 2019

Hey @Rishabh570, sorry I got a little busy over at ldi and forgot about this. Taking a look now!

@rexagod
Copy link
Member

rexagod commented Jun 1, 2019

@Rishabh570 I guess you should try resolving the promise before passing it down the chain, like this,

return fetch(`https://api.github.com/repos/${org}/${repo}/commits?since=${queryTime}`)
// add this =========              
  .then(function(res) {
                  if(response.status === "200") {
                    return Promise.resolve(res);
                  }
                })
// ===============
                .then(function gotResponse(response) {
                        return response.json();
                })

Let me know if this fixes it!

@Rishabh570
Copy link
Collaborator Author

@rexagod @jywarren Please visit http://rishabh570.me/community-toolbox/ . Now it's working fine...Here's the GIF,

REFACTOR_FINAL

[ I re-pushed it to my gh-pages ]

@Rishabh570
Copy link
Collaborator Author

Done with some final little changes regarding the recent contribs utility file name change...It's ready now I think...

@Rishabh570 Rishabh570 changed the title [ WIP ] Refactor the codebase Refactor the codebase Jun 2, 2019
@jywarren
Copy link
Member

jywarren commented Jun 3, 2019

Yes it works well! And it's fast! I'll merge this now, great work! Would you like merge rights to publiclab gh-pages or do you have that already?

@jywarren jywarren merged commit 30dd907 into publiclab:main Jun 3, 2019
@Rishabh570
Copy link
Collaborator Author

@jywarren No, I don't have those rights to PL's gh-pages currently...

@jywarren
Copy link
Member

jywarren commented Jun 6, 2019

Are you sure? I think you have write permissions for the project.

@Rishabh570
Copy link
Collaborator Author

I was a little confused actually...am I supposed to update publiclab/community-toolbox's gh-pages?

@jywarren
Copy link
Member

jywarren commented Jun 6, 2019 via email

@Rishabh570
Copy link
Collaborator Author

Rishabh570 commented Jun 6, 2019 via email

@jywarren
Copy link
Member

jywarren commented Jun 6, 2019 via email

TildaDares pushed a commit to TildaDares/community-toolbox that referenced this pull request Apr 28, 2021
* refactor the codebase.

* rename recent contributors utility file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor the codebase
3 participants