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

Add pagination; add missing fields and objects [#17] #18

Merged
merged 18 commits into from
Sep 6, 2017

Conversation

dnl-blkv
Copy link
Contributor

@dnl-blkv dnl-blkv commented Sep 3, 2017

No description provided.

@dnl-blkv
Copy link
Contributor Author

dnl-blkv commented Sep 3, 2017

@OGKevin when you're done with reviewing, please poke me but don't merge yet! :)

Copy link
Contributor

@OGKevin OGKevin left a comment

Choose a reason for hiding this comment

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

Ok, I did not do generated code right ? 😆 🙈

"Could not generate previous page URL params: there is no previous page.";

/// <summary>
/// URL Param constants.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I triple checked this time haha, not generated right 😝. Below you have param without capital P so I guess this one should also be lower case ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh OCD! :D I'll fix this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

private const int INDEX_PARAM_VALUE = 1;

/// <summary>
/// Base dummy URL to hack though the incomplete relative URI functionality of dotnetcore.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is though a typo ? Shouldn't it be through. And this is kinda clever btw 👏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's indeed a typo! Thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@OGKevin
Copy link
Contributor

OGKevin commented Sep 4, 2017

@dnl-blkv tests passing. please add pagination tests as we discussed and address the changes I proposed and it should be good to merge.

@dnl-blkv
Copy link
Contributor Author

dnl-blkv commented Sep 5, 2017

Cleanups done, Pagination test to go!

@OGKevin OGKevin modified the milestone: V0.11.0 Sep 5, 2017
@OGKevin
Copy link
Contributor

OGKevin commented Sep 5, 2017

All tests passing, LGTM. @andrederoos please take a look 👀.

Copy link
Contributor

@andrederoos andrederoos left a comment

Choose a reason for hiding this comment

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

same holds, review without generated will be better, rest is ok!

@andrederoos andrederoos merged commit d8f1950 into develop Sep 6, 2017
@OGKevin OGKevin deleted the 17-pagination branch September 6, 2017 18:39
@OGKevin OGKevin mentioned this pull request Sep 9, 2017
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.

3 participants