feat(cargo-shuttle): adaptive page hints, non-breaking #1334
Conversation
| ) | ||
| ); | ||
|
|
||
| if deployments.len() == limit as usize { |
There was a problem hiding this comment.
I like it! But I guess this will still be wrong when there are exactly limit projects, right? Could we change the limit logic to be limit + 1, then only display limit projects. So say there are 15 projects, we have a limit of 10 and we show those 10, but we got 11, so we know there are more projects on the next page.
There was a problem hiding this comment.
Oh, and a cargo fmt should fix CI
There was a problem hiding this comment.
I like it! But I guess this will still be wrong when there are exactly
limitprojects, right? Could we change the limit logic to belimit + 1, then only displaylimitprojects. So say there are 15 projects, we have a limit of 10 and we show those 10, but we got 11, so we know there are more projects on the next page.
Yes I did something similar to what you described in a separate PR in this commit:
25b382d
However I was instructed to change it to just return a record count vs a bool for the next page so I made that change and that PR still remains open here:
#1320
However the changes are breaking so I made this that can be merged without breaking.
There was a problem hiding this comment.
Oh I just reread this as I was checking up on the PR and I realized you meant doing the limit + 1 on the client-side, I can definitely change it to do that and as you mentioned it's 100% accurate whereas this can falsely hint at a next page, I was just doing it on the server-side not the client-side which caused it to be breaking.
There was a problem hiding this comment.
Yes, sorry for not making that clear! 😄 It would be great if you could implement this and resolve the conflicts, then I think we can get this merged for the next release.
|
Superseded by #1357 |
Description of change
Changed the project and deployment commands to only return page hint if the items returned == limit.
Mostly closes #1316
How has this been tested? (if applicable)
Tested with local setup