Skip to content
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

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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

ellinge
Copy link
Contributor

@ellinge ellinge commented Feb 8, 2023

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

image

@ellinge ellinge marked this pull request as draft February 8, 2023 21:29
Fixes iframe height calculation
Fixes sortable hover-style
@ellinge ellinge marked this pull request as ready for review February 8, 2023 22:57
@marisks
Copy link
Member

marisks commented Feb 17, 2023

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
Copy link
Contributor Author

ellinge commented Feb 17, 2023

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
Copy link
Contributor Author

ellinge commented Feb 17, 2023

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

@marisks
Copy link
Member

marisks commented Feb 17, 2023

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
Copy link
Contributor Author

ellinge commented Feb 18, 2023

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

@jevgenijsp
Copy link
Contributor

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.

#144

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No confirmation message when adding new redirect Feature request: Sortable headers in the Redirects tab
4 participants