Fix #6054: Hide fixed filter queries from search labels component - #6089
Fix #6054: Hide fixed filter queries from search labels component#6089guillermo-escire wants to merge 8 commits into
Conversation
alexandrevryghem
left a comment
There was a problem hiding this comment.
@guillermo-escire: I haven't tested this functionality yet, but this code recreates the observable during every change detection cycle, which is bad for performance. Could you address my feedback in the comments below?
| @@ -41,4 +47,20 @@ export class SearchLabelsComponent implements OnInit { | |||
| this.appliedFilters$ = this.searchService.appliedFilters$; | |||
| } | |||
|
|
|||
| get visibleFilters$() { | |||
There was a problem hiding this comment.
Please rewrite this to:
- Save the
fixedFilterQueryin aBehaviourSubject(fixedFilterQuery$) inngOnChanges - Remove the
visibleFilters$()getter - Create a new
visibleFilters()method that uses acombineLateston both thethis.searchService.appliedFilters$& thefixedFilterQuery$you created in step one. In the pipe of thiscombineLatestyou can then use the map you created in your oldvisibleFilters$()getter (I assume that this works properly)
There was a problem hiding this comment.
Done! I've refactored the component as suggested:
- Added
fixedFilterQuery$as aBehaviorSubjectupdated inngOnChanges - Removed the getter and replaced it with a
visibleFilters$Observable
initialized in ngOnInit using combineLatest
There was a problem hiding this comment.
@guillermo-escire: Thnx this looks much better now could you also add some inline docs explaining the purpose of the fixedFilterQuery$ & visibleFilters$
There was a problem hiding this comment.
Hi @alexandrevryghem , I've added inline docs for both fixedFilterQuery$ and visibleFilters$ explaining their purpose.
References
Fixes #6054
Description
Fixed filter queries were being displayed as visible broken badges in the search UI since DSpace v9, even though they are meant to be invisible to the user
Instructions for Reviewers
List of changes in this PR:
fixedFilterQuery@input toSearchLabelsComponentvisibleFilters$getter that filters out applied filters matching thefixedFilterQuerysearch-labels.component.htmlto usevisibleFilters$instead ofappliedFilters$search.component.htmlto passfixedFilterQuerytods-search-labelsHow to test:
/entities/person/<UUID>)Search.Filters.Applied.F.IsAuthorOfPublication: <UUID>no longer appearsChecklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.