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

Add section filter portlet #26

Closed
wants to merge 52 commits into from

Conversation

instification
Copy link
Member

@instification instification commented Dec 4, 2018

This PR adds a 'Location Filter' portlet which allows the user to filter content according to its position in the site.

It mimics the 'Navigation Portlet' but is filtered based on the contents of the collection.

I haven't added a corresponding tile, but I will do if it's required to get this PR merged.

Likewise the ajax updates aren't working. I might need some help debugging that, but it works ok reloading the page.

We need this functionality for a project and feel it would be better to improve this package rather than create a new package as there is a lot of shared concepts/code to produce the portlet.

Changes to make

  • Change portlet name from “Location filter” to “Section filter”
  • Add tests (unit and robot tests)
  • Add a tile

@instification
Copy link
Member Author

Some screenshots of how the portlet works:

screen shot 2018-12-04 at 16 10 27

screen shot 2018-12-04 at 16 10 37

screen shot 2018-12-04 at 16 10 46

@agitator
Copy link
Member

agitator commented Dec 4, 2018

Haven't tried out your pull request yet, but wouldn't something like a tree widget be a better fit?

iur

screenshot 2018-12-04 at 20 07 32

^ just a bit prettier ;-)

@instification
Copy link
Member Author

@agitator thanks for the feedback

For this initial version I decided to match the style of the navigation portlet so it is visually in keeping with other portlets. I don't know of any existing tree widget in Plone/Barceloneta and it would be a lot to try and implement something new. But if you had some ideas I will look at it.

However, the functionality of this initial version is limited, you can only filter down to a single folderish location. So a tree widget would need more work to implement. Also as it would need to execute more searches to build the tree, it might be costly.

@thet
Copy link
Member

thet commented Dec 5, 2018

I like the styling of the navigation portlet better than the tree widget.
I'll have a look later today.

@petschki
Copy link
Member

I like the location filtering feature.

But I think we could make the filter portlet/tile generation a bit more generic in general. In a project of us I needed a filter item for tree-like taxonomies (created by collective.taxonomy) and I could solve it partly with the IGroupByCriteria adapter but still had to jbot the portlet template. How about some kind of template attribute for the IGroupByCriteria adapter in general? Then you do not have to duplicate the get_filter_items code only for one index ... maybe we can get rid of search.py and collectionsearch.py also and only declare a filter_template for the index SearchableText ... But I'm just thinking out loud 😉

/cc @thet @agitator

@thet
Copy link
Member

thet commented Mar 25, 2019

@petschki Sounds all good.
@instification I like the location filtering feature too! Although that should be renamed to path filtering feature, as location is already used for a geographical location in Plone events. Sorry for not reviewing it yet. Currently I do have too little time for that.

@instification
Copy link
Member Author

@thet I don't have a problem renaming it to path, but I notice that on collections you can have a criteria of location which is the same.

Screen Shot 2019-03-27 at 10 49 12

So there seems to be a lack of consistency within Plone.

@instification
Copy link
Member Author

@petschki generally agree with you, but it seems like a more fundamental change to the package, and it's not something I'll have time to implement. Perhaps it might make more sense as a separate issue/feature request?

@thet
Copy link
Member

thet commented Mar 30, 2019

@instification I agree, there is some inconsistency.
For German, I did a re-translation from "Ort" (which is "location" in German) to "Pfad" ("path"): collective/plone.app.locales#125

@djay
Copy link
Member

djay commented Jul 9, 2019

@thet I agree that Location is overloaded so can be confusing but I think Path might be a little too technical and be related too much to how people understand Plone rather than websites. I'd suggest "Section" instead. Plone used to have a section filter in its quicksearch. Although it did only work for the first level of navigation I believe section applies to subsections and lower level paths too.

@djay djay force-pushed the instification/location_filter branch from 70c9c61 to c5100c9 Compare July 9, 2019 10:00
@djay
Copy link
Member

djay commented Jun 4, 2021

But I think we could make the filter portlet/tile generation a bit more generic in general. In a project of us I needed a filter item for tree-like taxonomies (created by collective.taxonomy) and I could solve it partly with the IGroupByCriteria adapter but still had to jbot the portlet template. How about some kind of template attribute for the IGroupByCriteria adapter in general? Then you do not have to duplicate the get_filter_items code only for one index ... maybe we can get rid of search.py and collectionsearch.py also and only declare a filter_template for the index SearchableText ... But I'm just thinking out loud 😉

I don't think its that similar. Currently doing a filter of "Location" doesn't seem to work at all.
Maybe there is an argument for combining it into the filter portlet for when users pick "location' but it would still need to do the query specially to group by parent folders and change the way it displays it add indenting. Maybe if taxonomies also used path indexes then it could share the same style? - but note that this section filter displays parents/sections, not full paths.

In terms of adding "templates", I think there is already too much technical customisation features in the UI for something that should be end user friendly.

Is it possible to get this reviewed so we can merge it?

@djay
Copy link
Member

djay commented Jul 2, 2021

The more I look at the code, the more I think it can be handled with some modifications to the IGroupByModifier mechanism.

  • Currently there is a path index (Location... still a bad name) which doesn't seem to be well handled by the collection filter well
  • instead of treating a path as a single value to group, we break down a into all the parents (get rid of node path) and grouby these
    • so need a new modifier to turn metadata value to 1 or more groubing values
  • alter the templates (or the groupbymodifer) to allow able to display a path as indented (and just include parent)
    • should be possible for links, tickbox, radio and dropdowns.
    • maybe some way to make it look like navigation box by default?
  • No more section filter or new portlet type
    • User creates a filter portlet and selects "Location" as the index and it just works with indented parents
    • Still prefer section to location as a name however. Maybe relabel the groupby?

@djay djay changed the base branch from master to collectionish July 8, 2021 11:48
Base automatically changed from collectionish to master July 19, 2021 11:46
@djay
Copy link
Member

djay commented Nov 17, 2021

replaced by #136

@djay djay closed this Nov 17, 2021
@jensens jensens deleted the instification/location_filter branch December 9, 2021 17:34
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.

8 participants