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

Disable _all_ filter option #138

Open
wants to merge 115 commits into
base: 3.x
Choose a base branch
from
Open

Disable _all_ filter option #138

wants to merge 115 commits into from

Conversation

JeffersonBledsoe
Copy link
Member

@JeffersonBledsoe JeffersonBledsoe commented Sep 6, 2021

This PR adds an option to the collection filter settings panel to disable the 'All' option from the filter list. In order to provide a default filtering fallback, this PR also adds a redirect to the current page with collectionfilter=1 added as a query string if the collectionfilter is about to be displayed but the query string is missing. This is because the filtering of the collection is done by an event handler for each page traversal on the site. When the all option is disabled, we will use the first option from the filter as the default filter.

Closes #137

MrTango and others added 30 commits May 19, 2021 18:23
Add idx parameter to display_modifier call
implement AJAX geoJSON feature
Revert "implement AJAX geoJSON feature"
@JeffersonBledsoe JeffersonBledsoe marked this pull request as ready for review December 2, 2021 09:41
@JeffersonBledsoe JeffersonBledsoe requested review from agitator and petschki and removed request for djay December 2, 2021 09:41
@JeffersonBledsoe JeffersonBledsoe changed the title WIP: Disable _all_ filter option Disable _all_ filter option Dec 9, 2021
@JeffersonBledsoe
Copy link
Member Author

@petschki Any chance you could have a look over this?

Copy link
Member

@petschki petschki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response. While it makes sense to disable the all option for filteritems, there must be a better way to inject the first filteritem as default filtering than redirecting again to the same page with collectionfilter=1 in query. This is a major performance downgrade of the whole package and I'm against merging it into master. @thet @agitator any thoughts?

@djay
Copy link
Member

djay commented Dec 16, 2021

@petschki it shouldn't be a performance impact most of the time. The whole reason to do the redirect is avoid performance problems.
The only time the redirect happens is if the filter with "all" disabled appears the first time without collectionfilter=1. After that there is no cost. If the All is not disabled there is no cost. On other parts of the site there is no cost.

If you can think of another way to achieve this without the redirect then great but we did think about it and couldn't come up with another way. The nature of collectionfilter is that portlets can't influence the query until it's hit the collection for first time. Not without monkey patching collections and centrally storing the filter definitions outside of the portlets and onto the collection itself.

@djay
Copy link
Member

djay commented Jan 13, 2022

@petschki any update on this?

@JeffersonBledsoe
Copy link
Member Author

Hi @petschki, any chance you've had any more thoughts on how it would be possible to inject collectionfilter=1 into the query without redirecting? As @djay mentioned, there should not be any performance impact most of the time, but you're right in that it would be great to avoid this completely. I'm working on a few changes to this PR and would be great if there were a way to get this merged eventually!

@JeffersonBledsoe JeffersonBledsoe requested review from petschki and MrTango and removed request for agitator March 7, 2022 21:57
run: |
pip install black
pip install black==21.12b0
Copy link
Member Author

@JeffersonBledsoe JeffersonBledsoe Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petschki Had to pin the version of black to the one before the stable release as they dropped Python2.7 support (e.g. formatting tries to remove all the u characters). Happy for this to be in this PR, or want it in a separate one?

@petschki
Copy link
Member

Hi, I've quickly tried this feature in a project of ours and found out the following:

  • if there's only one filteritem on a page with default_filter_behavior="Select first" it works (only tested the portlet)
  • if there is a second filteritem (no default filter behavior set), I get redirected to the page with ?collectionfilter=1 only ... the first filteritem has the first value selected, but the listing is not constrained to the first value.

Does this work with multiple group_by filteritems or did I do something wrong?

@petschki petschki changed the base branch from main to 4.x June 20, 2022 13:16
@petschki petschki changed the base branch from 4.x to 3.x June 20, 2022 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No way to disable the 'All' option for filtering
6 participants