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.

Sign up for GitHub

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

Jump to bottom

feat(filter): enable multi-selection filter #2173

Merged
merged 12 commits into from
Jan 15, 2024
Merged

feat(filter): enable multi-selection filter #2173

merged 12 commits into from
Jan 15, 2024

Conversation

tomwwinter
Copy link
Contributor

@tomwwinter tomwwinter commented Jan 10, 2024 •
edited by TheSlimvReal
Loading

closes: #2075

Still some points open:

  • test coverage
  • some TODOs in the code

Visible/Frontend Changes

  • filter behaviour -> multi select instead of of single select
  • [ ]

Architectural/Backend Changes

  • [x]
  • [ ]

@github-actions GitHub Actions
Copy link

Deployed to https://pr-2173.aam-digital.net/

TheSlimvReal
TheSlimvReal reviewed Jan 11, 2024
View reviewed changes
Copy link
Collaborator

@TheSlimvReal TheSlimvReal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice work already. Feels already very intuitive.

I only discovered one functional issue

  • in roll call (record attendance) all options are pre-selected and entries without the property are not shown on default. I think this should just no create any filter object on default.

src/app/core/filter/filters/filters.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
src/app/core/basic-datatypes/date/date-range-filter/date-range-filter.component.spec.ts Outdated Show resolved Hide resolved
src/app/core/filter/filter-generator/filter-generator.service.ts Show resolved Hide resolved
src/app/core/filter/filter/filter.component.ts Outdated Show resolved Hide resolved
src/app/core/filter/filters/filters.ts Outdated Show resolved Hide resolved
src/app/core/filter/filters/filters.ts Show resolved Hide resolved
src/app/core/filter/filters/filters.ts Outdated Show resolved Hide resolved
src/app/core/filter/list-filter/list-filter.component.html Show resolved Hide resolved
src/app/core/filter/list-filter/list-filter.component.ts Show resolved Hide resolved
@sleidig
Copy link
Member

sleidig commented Jan 11, 2024

I did a functional / UX review:

  • you had suggested a small "select all | clear all" option at the top of the dropdown. I think this would still be a very useful addition
  • Do we have an option to disable multi-select? The TodoList special filter doesn't make that much sense in combinations (although it is not a huge issue either). Users here would almost always want to switch from one of the custom filters to the other ("all open" to "completed"), not select one in additional to the existing.
  • some console.log is still happening ;-)

@tomwwinter tomwwinter force-pushed the tw/feat/APP-1601-multi-status-filter branch from 8995d09 to e7144b8 Compare January 11, 2024 12:50
@tomwwinter tomwwinter force-pushed the tw/feat/APP-1601-multi-status-filter branch from e7144b8 to bc71301 Compare January 11, 2024 12:50
TheSlimvReal
TheSlimvReal requested changes Jan 11, 2024
View reviewed changes
Copy link
Collaborator

@TheSlimvReal TheSlimvReal left a comment

Choose a reason for hiding this comment

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

Thanks for already integrating the things I pointed out. Functionality works very well. Only one more thing I discovered:

  • default filter values in tasks and notes list are not working anymore. You can check at the deployed demo to see the difference

Generally, it would be very helpful if you don't overwrite your commits after a review has already been done. This way I don't know what new changes have been made to a file and there is no way of finding that out so I have to check the whole file again. Also locally I have to remove your branch locally to pull the updates.

src/app/core/basic-datatypes/configurable-enum/configurable-enum.service.ts Outdated Show resolved Hide resolved
src/app/core/filter/filter.service.ts Show resolved Hide resolved
src/app/core/filter/filters/booleanFilter.ts Show resolved Hide resolved
src/app/core/filter/filters/entityFilter.ts Show resolved Hide resolved
src/app/core/filter/filters/filters.ts Show resolved Hide resolved
src/app/core/filter/filter/filter.component.ts Outdated Show resolved Hide resolved
src/app/core/filter/filter/filter.component.ts Outdated Show resolved Hide resolved
src/app/core/filter/filters/filters.ts Outdated Show resolved Hide resolved
src/app/core/filter/filters/dateFilter.ts Outdated Show resolved Hide resolved
tomwwinter and others added 5 commits January 15, 2024 14:23
@tomwwinter
Merge branch 'master' into tw/feat/APP-1601-multi-status-filter
bcdf20f 
# Conflicts:
#	src/app/core/filter/filter.service.ts
#	src/app/core/filter/filter/filter.component.ts
#	src/app/core/filter/filters/filters.ts
#	src/app/features/todos/todo-list/todo-list.component.ts
@tomwwinter
fix: use correct import paths
deb0262
@TheSlimvReal
small code cleanups
6f7a9cb
@TheSlimvReal
removed status prebuilt filter
1c38fd9 
REQUIRES MIGRATION
@TheSlimvReal
more code cleanups
f9d9ae2
TheSlimvReal
TheSlimvReal requested changes Jan 15, 2024
View reviewed changes
Copy link
Collaborator

@TheSlimvReal TheSlimvReal left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanups and tests. Looks great now. I did some smaller code changes, mostly unnecessary variables and usage of let instead of const when the variable is never re-assigned. We generally try to use const as much as possible to prevent side-effects.
I have also removed a prebuilt filter from the NotesManagerComponent as this can now be done with a normal filter on warningLevel directly. We will need a small config migration for this though.

src/app/core/filter/filter-generator/filter-generator.service.spec.ts Outdated Show resolved Hide resolved
src/app/core/filter/filter-generator/filter-generator.service.spec.ts Outdated Show resolved Hide resolved
src/app/core/filter/filter.service.ts Outdated Show resolved Hide resolved
@tomwwinter tomwwinter merged commit 52c1696 into master Jan 15, 2024
7 checks passed
@tomwwinter tomwwinter deleted the tw/feat/APP-1601-multi-status-filter branch January 15, 2024 16:40
@aam-digital-ci
Copy link
Collaborator

🎉 This PR is included in version 3.29.0-master.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@aam-digital-ci aam-digital-ci added the released on @master managed by CI (semantic-release) label Jan 15, 2024
@aam-digital-ci
Copy link
Collaborator

🎉 This PR is included in version 3.29.0-master.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@aam-digital-ci
Copy link
Collaborator

🎉 This PR is included in version 3.29.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@aam-digital-ci aam-digital-ci added the released managed by CI (semantic-release) label Jan 17, 2024
sleidig pushed a commit that referenced this pull request Jan 22, 2024
@tomwwinter @sleidig
feat(filter): enable multi-selection filter (#2173)
c5de85a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheSlimvReal TheSlimvReal TheSlimvReal approved these changes

@sleidig sleidig Awaiting requested review from sleidig

Assignees
No one assigned
Labels
released on @master managed by CI (semantic-release) released managed by CI (semantic-release)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

multi-status filter (show both entities matching either status A or status B)
4 participants
@tomwwinter @sleidig @aam-digital-ci @TheSlimvReal