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

HAL #34

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

HAL #34

wants to merge 19 commits into from

Conversation

petteraas
Copy link
Contributor

Please don't just pull this in yet, as it's so far just a starting point, I will need to do a lot more work with it, but I wanted to share it with you early on to get some feedback.

The library 'express-hal' which I'm using here doesn't appear to be maintained, but it's a fairly short piece of code, I'm thinking that we can fork it and maintain it if we decide to use it.

@petteraas
Copy link
Contributor Author

It's starting to look like a working backend now, but still the front end needs to be rewritten to handle the new HAL formatted JSON.

.then(function (result) {
res.json({'results': result, 'total': result.total});
var links = {
self : '/retrospectives?page=' + page + '&limit=' + limit,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this string concat could be wrapped in a util function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, and I'll look into moving the handling of limit+page+start somewhere else as well, so it's not duplicated both here and in getTickets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added lib\paginator with methods to handle 'item links' and 'bulk links' and the creation of 'start'

limit = 10,
page = 1;

if (req.query.page) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should probably add a check that req.query.page is within 1 and N where N is top most page for the result, but I think that belongs in another review.

The way it behaves now, if you have say 10 pages, and you send in ?page=30 it will create a previous link pointing to page=29 instead of redirecting you to page=10 and creating previous:9 which is probably a cleaner solution, but I consider this a edgecase for now.

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