Keeps search state on post, allow order by column

Geta / geta-notfoundhandler

The popular NotFound handler for ASP.NET Core and Optimizely, enabling better control over your 404 page in addition to allowing redirects for old URLs that no longer works.

Apache License 2.0

20 stars 16 forks source link

Keeps search state on post, allow order by column #80

Open ellinge opened 1 year ago

ellinge commented 1 year ago

Partial #73 Fixes #117

I've rewritten the fetching for suggestions and redirects to allow it to store search state on delete / page change.

I also added the possibilty to sort the columns.

I also added pagination to the sql-fetching ~~and by that also removed dependency to X.PagedList~~. Dependency on PagedList kept for backward compability.

Added confirmation message on operations, which fixes #13

marisks commented 1 year ago

I have looked at the PR and it has several significant issues:

There are a lot of breaking changes - public interfaces have been changed. We do not want to introduce breaking changes at this point.
Using POST to get/query data is a bad practice. Instead, it should use links with query parameters.
Custom implementation of Paging - there was already an X.PagedList library in use that has it implemented and tested.

Because of these issues, I cannot accept this PR, but it could be a reference for a new PR without these issues. Although, I would discuss first how it should be implemented in the issue #73 before doing anything.

ellinge commented 1 year ago

The problem with current implemenation, in addition to the bad user experience, is that you fetch all posts instead of pagination on database side. A removed third party dependency for just calculating number of pages is not such a bad thing.

Regarding public interfaces. Current methods can be kept with a redirect to the new implementation. One would have to keep paged list then though and remap a ”fetch all” to this.

If you want to keep state with get state needs to be kept in sync manually for all different kinds of filters/query you add. Using post all together makes this work seemlessly. But probably can keep this in sync with js then. Aka updating hidden field with current query. One perhaps can wrap it with a get-form and trigger the posts in a different way without introducing any form nesting…

ellinge commented 1 year ago

If I tried to work around these issues could you have another look or is that just wasted time?

marisks commented 1 year ago

Maybe the best would be so that this PR would be a reference for multiple other PRs. I will go through the PR and comment on parts that should be pushed separately. And also could comment on parts of how to change public interfaces and other stuff.

ellinge commented 1 year ago

Now public interfaces are restored and GET:s and POST:s are separated.

jevgenijsp commented 3 weeks ago

As mentioned earlier, we should use this PR as a reference for multiple smaller PRs. Inspired by your work, I moved the sortable headers into a separate PR and adjusted the code.

https://github.com/Geta/geta-notfoundhandler/pull/144