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
Content list owner filter #15470
base: main
Are you sure you want to change the base?
Content list owner filter #15470
Conversation
src/OrchardCore.Modules/OrchardCore.Contents/Views/ContentsAdminListFilters.cshtml
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs
Outdated
Show resolved
Hide resolved
query.Where(x => x.NormalizedUserName.Contains(_userManager.NormalizeName(searchTerm))); | ||
} | ||
|
||
var users = await query |
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.
We may need to cache this for optimization otherwise we'll significantly increase the server load.
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.
I had the same thought. It at least isn't page blocking with it loading after page render. I was thinking it would be fine if it would only start loading users when the select dropdown is shown, but bootstrap-select does not make that easy. Thoughts?
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.
@douwinga we agree on your suggestion to only load the list on the first drop down, and maybe less than 50, probably something close to what the drop down can fit by default.
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.
If "Owner" is indexed, you may also not have to load all users, but just the distinct values of this field.
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.
@douwinga a better idea Sebestien mentioned was to not load any users by default. If the user clicks on the filter menu, then make an AJAX request that would fetch the first 50 users as you are doing today. This way you don't need to cache anything and also you don't need to render anything on every single request.
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.
@MikeAlhayek ya, I am just struggling to implement that with bootstrap-select
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.
let me know what you think. I made it load the data on first dropdown toggle. It is gross, but I am not sure how else to do it. bootstrap-select needs help, but I want to use it for this so that it is consistent with all the other filter dropdowns,
Is the PR ready for review? |
I set it to draft because I am not really happy with the hack to get around issues with bootstrap-select. I started a completely different approach that won't be a hack and give more control. I was waiting for any opinions before I put more work in. There are a few options that I am seeing. A) use my current idea which is just updating the options ourselves rather than using the less than ideal feature of bootstrap-select B) adding another library just for this one drop down C) replace bootstrap-select everywhere with a new library D) just remove the drop down completely and have users use the search bar with owner:... E) fix bootrap-select and hope the PR gets merged. |
option A) mentioned above does not work due to a bug in bootstrap-select v1.14.0-beta3 that makes it not possible to clear the option which is needed for searching the users by term. Rolling back to a version before beta appears to not have the bug. |
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Pleaes comment if you'd like to pick it up and remove the "stale" label. |
Adds content list owner filter requested in issue #14343