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

[F] New paginated components route #48

Merged
merged 28 commits into from
Jan 5, 2019
Merged

[F] New paginated components route #48

merged 28 commits into from
Jan 5, 2019

Conversation

DennisSchwartz
Copy link

This will be used by the frontend for infinite component scroll. It's not affecting any existing functionality.

Copy link
Contributor

@yochannah yochannah left a comment

Choose a reason for hiding this comment

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

@DennisSchwartz A couple of comments at this point:

  1. Docs: Could you add instructions how to enable and disable logging? Apologies if I've missed it somewhere
  2. Docs again: GITHUB_CLIENT_SECRET and ID - presumably users should set these somewhere - can we write down where?
  3. It would be a little easier in the future to work with a forked branch rather than develop on the main repo, but this should be fine for now.

@yochannah
Copy link
Contributor

To test the endpoint, should I GET something like http://localhost:8000/components?page=1&size=40 ?

@DennisSchwartz
Copy link
Author

@yochannah Yes that get request should work. You can also run it together with this: biojs/biojs-frontend#63 To see the effect in the frontend.

@DennisSchwartz
Copy link
Author

DennisSchwartz commented Jan 3, 2019

Docs: Could you add instructions how to enable and disable logging? Apologies if I've missed it somewhere

Could do, at the moment it's disabled like it was before, but you can enable it manually during development, so I didn't bother with a README entry.

Docs again: GITHUB_CLIENT_SECRET and ID - presumably users should set these somewhere - can we write down where?

This one is a bit tricky, I'm using a personal Github access token for development. But I agree, I will add some docs for that.

It would be a little easier in the future to work with a forked branch rather than develop on the main repo, but this should be fine for now.

Yep, but I got confused by the develop and master branches and they weren't in the proper states anyway. I'll introduce a bit of a different branch structure (that I think is more commonly used) once I add the automated deployment.

@DennisSchwartz
Copy link
Author

@yochannah I've added documentation for your points 1. and 2.

@yochannah
Copy link
Contributor

@DennisSchwartz The docs look great! <3 nice one.

I'm trying to preview on http://dev.biojs.net/?#/components - is that where I should see infinite scroll? Tried on FF and Chrome but nothing happens at the end of the page.

@DennisSchwartz
Copy link
Author

@yochannah Well the infinite scroll is not deployed on dev in the frontend yet, because it's still a PR (biojs/biojs-frontend#63) but I'm just gonna merge that one now haha.

@yochannah
Copy link
Contributor

yochannah commented Jan 4, 2019 via email

@DennisSchwartz DennisSchwartz merged commit 9f27fd1 into master Jan 5, 2019
@DennisSchwartz DennisSchwartz deleted the develop branch January 5, 2019 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants