Page reset762

[Avd6977]

Fix for #762

[coveralls]

Coverage increased (+0.0001%) to 74.963% when pulling 37266ad on Avd6977:PageReset762 into 6740f71 on gregnb:master.

[gabrielliwerant]

Seems to me like this PR would cause a regression with this: #647.

If the page is set, and we don't reset it on searches (and filters and probably some other things), then the results may not show and we may get errors. The page has to be reset on searching.

[gabrielliwerant]

What we need to know here is two things:

1. Whenever the searchText goes from empty to something, we need to reset page to 0
2. If the previous searchText is not empty, then we need to make sure the page number does not exceed the list of results (something like RoundUp(results / rowsPerPage) = max page number.

[Avd6977]

Why don't we just not reset it and have that be the responsibility of the consumer? If they're passing in page and search text, then they know that the page should be reset and can simply add page = 0 when search text is changed (that's what we're doing and how we caught this). So if the consumer is managing the page state, then there shouldn't be a need to reset it.

[gabrielliwerant]

I understand your point, but it would be unclear to me that I would need to reset the page after a search. I would expect searching to handle this for me. In particular, there are definitely issues with out of bounds pages after searching that should at least be checked against occurring, and then handled appropriately.

I'd also rather not go back on fixes that existed previously if at all possible (there are exceptions, but I don't think this should be one). People are relying on the fixes that were put in place previously, so I don't want to break it again. I think with additional logic, we should be able to achieve both aims here. If you like, I can help out here when I get a chance.

[Avd6977]

Yeah, I understand not wanting to revert back a bug. My thought was that the whole project was moving away from managing the internal state and moving toward putting more onus on the consumer of the table. Some documentation to reset page when searching or filtering may just be needed? Does the paging update when the filter is changed, as well?

Could be documented as a breaking change, as well. But if you'd rather put it in the code to fix this, that's fine. What would be your ideas to do that and not break the old bug? I'm not sure exactly what you're wanting to fix it with so some help would be great.

[gabrielliwerant]

Sorry, it's been a while, but if you're up for it, I have a suggestion now that I think will satisfy both use cases.

[gabrielliwerant]

Ok, how about !this.props.options.serverside instead of !this.props.options.page? Same for below. That way, we keep the old bugfix for people that are using custom pages, but we prevent serverside behavior from changing. @Avd6977

[Avd6977]

That should work for us and keep the serverside behavior the same, but would it be confusing for anyone using it? I made the change so if we're good with it you can just accept it.

[gabrielliwerant]

Yep, this should work just fine, as it looks like searches on the client are disabled id serverSide === true, meaning that no one using serverSide === true should experience a regression. Thanks for making the change and for bearing with me!