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

Allow user to filter lists #5035

Merged
merged 25 commits into from Mar 20, 2020
Merged

Conversation

danrot
Copy link
Contributor

@danrot danrot commented Feb 5, 2020

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets fixes #4855
Related issues/PRs ---
License MIT
Documentation PR sulu/sulu-docs#prnum

What's in this PR?

This PR adds a new feature to filter list for values in certain columns.

Why?

Because we need that in quite a few projects.

Example Usage

BC Breaks/Deprecations

  • The ListBuilderInterface has a new filter method

To Do

  • Create a documentation PR
  • Add breaking changes to UPGRADE.md
  • Check if filters also work if they are not part of the selected fields
  • Check if sortFields really need to be iterated over in findByIdsGivenCriteria method, because it seems to be done another time in assignSortFields
  • Check if tagIds e.g. in contact.xml, webspaceKeys in target_groups.xml or tagIds in accounts.xml list configuration is possible. It is quite hard, because the ListBuilder builds a GROUP_CONCAT based on that field. The Selection filter type however would still work for 1:n relationships, but this one is n:m...
  • Should filter work on trees? (mainly pages and categories)
  • How to handle PHPCR content? (pages, snippets, custom-urls, analytics)
  • Check if some filters should be removed because they don't make any sense
  • MultiAutoComplete should take SelectionStore as argument to avoid multiple requests in SelectionFieldFilterType

Functional todos:

  • Design of Chip component
  • Where to place filter chips
  • Positioning of buttons (search, column options and filter)
  • Use auto-complete instead of MultiSelection
  • Search on enter in text field and dates (?)
  • Show value in chip
  • Don't show value in chip before input has blurred, OK button has been pressed or whatever
  • Dropdown for media (type, mimeType)?
  • Return promise in getValueNode function to allow resolving of IDs for SelectionFieldFilterType?
  • How to get old value (e.g. to merge object in DateFieldFilterType and NumberFieldFilterType
  • Partial and exact match for TextFilterType
  • Add filter information to URL in order to remember what has been selected after back button click
  • Double requests when date filters are used
  • Search button should also be left if no filters are available
  • Excerpt returns tag ids as strings, causing a reaction to execute and trigger the save button immediately after opening the form
  • SmartContent does not filter correctly when tags have already been stored before (sends the tag itself instead of the id in the items request for the form) (related to SmartContent - numeric Tags not working #4358?)

@danrot danrot force-pushed the feature/list-filter branch 5 times, most recently from 6744be1 to 7f3f24f Compare February 7, 2020 12:49
@lightglitch
Copy link
Contributor

lightglitch commented Feb 13, 2020

It's looking good. It's suppose to solve #4855?

@danrot
Copy link
Contributor Author

danrot commented Feb 14, 2020

@lightglitch Thank you for reminding me of that issue, yeah, this will be solved by this 🙂

@danrot danrot force-pushed the feature/list-filter branch 2 times, most recently from a9d7b4e to 8310b14 Compare February 19, 2020 16:30
@danrot danrot force-pushed the feature/list-filter branch 7 times, most recently from 9c8b71d to 5f8d061 Compare February 27, 2020 09:23
@chirimoya
Copy link
Member

chirimoya commented Feb 27, 2020

@danrot Nice Job! Personally, I would prefer one-liners and a real Buttons. Also depends on the decision regarding the label.

  • Store filter for user session
  • Contact Birthday filter didn't work
  • Contact Organization filter didn't work
  • Media Type filter didn't work
  • Missing filters for Snippets & Categories
  • Date format changes... see screen shot

Bildschirmfoto 2020-02-27 um 15 21 07

@danrot danrot force-pushed the feature/list-filter branch 2 times, most recently from 6f624c4 to bf72e9e Compare March 3, 2020 06:06
@niklasnatter
Copy link
Contributor

i am not able to remove a boolean filter that is set to false if it was set to true before 🤔

@niklasnatter
Copy link
Contributor

niklasnatter commented Mar 20, 2020

The following things should probably be addressed in another pr:

  • selection filter looks broken if i select more than one entity (eg two different titles in contact list)
  • dropdown popover of the dropdown filter covers the OK button when filtering meda types. i can only close it by clicking somewhere outside of the filter chip
  • allow to select only a from date or a to date in the datetime filter (right now this leads to a failed request)

@niklasnatter
Copy link
Contributor

niklasnatter commented Mar 20, 2020

Furthermore, i would propose the following usability improvements:

  • close the filter chip with ESC
  • confirm the filter chip with ENTER
  • focus the text field automatically when i open a filter chip
  • display the value of the number filter on the same line as the operator
  • display a placeholder (eg with an example) in the text fields

@niklasnatter
Copy link
Contributor

niklasnatter commented Mar 20, 2020

Apart from these small quirks, the functionality feels great and integrates beautifully into the UI. I think our users will really like this feature! 🙂

I would propose to merge this PR if the boolean filter bug is resolved and implement the other issues in another PR. Is this okay for you? @sulu/core-team ? would be nice to get this huge thing out of the way.

@danrot
Copy link
Contributor Author

danrot commented Mar 20, 2020

So to summarize:

@danrot
Copy link
Contributor Author

danrot commented Mar 20, 2020

The dropdown issue is (of course) a z-index issue... Don't know how to quickly fix that... I would love to get rid of all z-indexes all together, they always cause trouble at some point... I am checking the other issues right now, maybe I can fix one or the other before merging this PR, shouldn't be too bad.

Copy link
Contributor

@niklasnatter niklasnatter left a comment

Choose a reason for hiding this comment

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

thank you! would be nice if confirming/closing the filter chip would work even if the input field is currently focused. i guess the problem here is something similar to #5054

anyway, a lot of things are working great and therefore i am happy to finally merge this 🙂

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.

Sulu 2 admin filters
4 participants