Paginate My Shots #3657
Paginate My Shots #3657
Conversation
| const queryParams = {}; | ||
|
|
||
| Object.keys(searchKeys).forEach(x => { | ||
| if (typeof model[searchKeys[x]] !== "undefined" && model[searchKeys[x]] !== null) { |
There was a problem hiding this comment.
You could do model[searchKeys[x]] !== undefined here.
| let qs = queryString.parse(window.location.search) | ||
|
|
||
| Object.keys(searchKeys).forEach(x => { | ||
| if (typeof qs[x] !== "undefined") { |
There was a problem hiding this comment.
You could do qs[x] !== undefined here – typeof is only necessary for top-level variables (e.g., typeof someVar !== "undefined")
| refreshModel(); | ||
| }; | ||
|
|
||
| exports.onChangePage = function(pageNumber) { | ||
| pageNumber = (pageNumber && parseInt(pageNumber)) || 1; |
There was a problem hiding this comment.
some linter somewhere is apt to warn about not using parseInt(pageNumber, 10) here. Though I'm unsure why it needs parseInt at all? Seems like it should pass through as a normal number.
There was a problem hiding this comment.
Good catch. I think it's the remnants of earlier code that I didn't update after some refactoring. At one point the page number could also come from the query string, making it a string instead of a number.
👍
| } else if (!queryParam[x] && qs[x]) { | ||
| delete qs[x]; | ||
| } | ||
| }); |
There was a problem hiding this comment.
I don't understand why this is updating qs? Couldn't you just use queryParam below to construct the query string?
There was a problem hiding this comment.
Poor naming on my part. The queryParam argument contains only the key/value pair(s) to update. I'll rename and add a comment.
| }); | ||
|
|
||
| if (Object.keys(qs).length) { | ||
| let newQueryString = Object.keys(qs).map(x => `${x}=${encodeURIComponent(qs[x])}`).join('&'); |
There was a problem hiding this comment.
nit: the name should also be quoted, i.e., ${encodeURIComponent(x)}
There was a problem hiding this comment.
D'oh! This should've been updated to use stringify from query-string.
👍
| function updateHistory(queryParam) { | ||
| let url = exports.getNewUrl(queryParam); | ||
| window.history.pushState(null, "", url); | ||
| } |
There was a problem hiding this comment.
Does this work with search parameters too? My impression from this code is that it's meant to handle the two parameters at the same time. To enable search you can set DISABLE_SEARCH="", then /shots?q=something will search. Actually I think it'll work regardless, we only hide the search input box, so you can edit the URL and do a search that way. Anyway, would be good to test together.
There was a problem hiding this comment.
Yes. I had the search feature enabled while I was working on this. 👍
server/src/pages/shotindex/server.js
Outdated
| function _render(shots) { | ||
| req.shots = shots; | ||
| function _render(shotsPage) { | ||
| Object.assign(req, shotsPage); |
There was a problem hiding this comment.
This makes me a bit uncomfortable, because there's arbitrary attributes being added to the request now. I'd rather req.shotsPage = shotsPage or else explicitly assigning the individual variables.
server/src/pages/shotindex/view.js
Outdated
| @@ -87,6 +88,62 @@ class Body extends React.Component { | |||
| </div>; | |||
| } | |||
|
|
|||
| renderPageNavigation() { | |||
| if (parseInt(this.props.totalShots) === 0) { | |||
server/src/pages/shotindex/view.js
Outdated
| } | ||
|
|
||
| let totalPages = Math.ceil(this.props.totalShots / this.props.shotsPerPage); | ||
| let prevLink = () => { |
There was a problem hiding this comment.
Why are prevLink and nextLink functions?
There was a problem hiding this comment.
They certainly don't need to be. 👍
server/src/servershot.js
Outdated
| @@ -15,6 +15,9 @@ const PNG_HEADER = Buffer.from(PNG_HEADER_BASE64, "base64"); | |||
| const JPEG_HEADER_BASE64 = "/9g="; | |||
| const JPEG_HEADER = Buffer.from(JPEG_HEADER_BASE64, "base64").slice(0, 2); | |||
|
|
|||
|
|
|||
| const SHOTS_PER_PAGE = 10; | |||
There was a problem hiding this comment.
10 seems like a small number. I think 24 would be better. Also divisible by 2, 3, or 4 columns. Well, our current flow isn't a real grid, but I think we intend to turn it back into a grid.
|
Note: for little stuff like this, sometimes we keep track / acknowledge the issues by thumbsuping each comment as you address it. |
|
This is ready to land, but I'm going to wait a bit on it so it's not part of the next server release |
Fix #3391