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

Content list owner filter #15470

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

douwinga
Copy link
Contributor

@douwinga douwinga commented Mar 7, 2024

Adds content list owner filter requested in issue #14343

  • Only loads 50 users at a time
  • Auto loads more users as you scroll via AJAX requests
  • Searches user options via AJAX request as well
  • Adds new filter syntax for owner:...

owner-filter

@douwinga douwinga changed the title Feature/owner filter Content list owner filter Mar 7, 2024
query.Where(x => x.NormalizedUserName.Contains(_userManager.NormalizeName(searchTerm)));
}

var users = await query
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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,

@douwinga douwinga marked this pull request as draft March 8, 2024 03:54
@hishamco
Copy link
Member

Is the PR ready for review?

@douwinga
Copy link
Contributor Author

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.

@douwinga
Copy link
Contributor Author

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.

Copy link

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.

@github-actions github-actions bot added the stale label May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants