-
-
Notifications
You must be signed in to change notification settings - Fork 279
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
Pay down some tech debt #465
Conversation
this.setState({from: 0, query: fullQuery}); | ||
this.setState({query: fullQuery}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be removed. I can see why you did this but actually there is a bug which needs to be fixed first.
Umm. If you go to this https://test.bookbrainz.org/search?q=harry+potter&from=10 , you'll notice the pagination is showing 1-20
. It should be showing 11-30
I checked and saw from
is missing in the props here
const props = generateProps(req, res, {
entityTypes,
from,
hideSearch: true,
nextEnabled,
resultsPerPage: size,
...searchResults
});
Then this change won't be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on IRC, from
is not needed in the state.
There is another issue that will be solved in a separate PR (#466 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew, nice one !
The technical debt gods are assuaged.
Thanks for the cleanup ! Can I go ahead and merge?
Problem
There's a fair bit of tech debt lying around.
Solution
Take an initial crack at paying it down. Delete unused or duplicated code. Clean up lints. Update some doc comments. Do some style work. Introduce subtle yet horrifying bugs.
Areas of Impact
General.