search: keep only the children filter and add filter list #503

vgranata · 2022-06-20T07:11:56Z

Signed-off-by: Johnny Mariéthoz Johnny.Mariethoz@rero.ch

Removes the parent filters when a filter is selected in a hierarchical
aggregation.
Removes useless methods.
The aggregationsExpand configuration can be a function.
Adds indeterminate state to the parent filters.
Stores the aggregationKey and parent properties in each bucket.
Add filter list
Removed ngx-bootstrap-slider
Display banner no results in current page

Co-Authored-by: Johnny Mariéthoz Johnny.Mariethoz@rero.ch
Co-Authored-by: Valeria Granata valeria@chaw.com

Screenshot

Why are you opening this PR?

Which task/US does it implement?
Which issue does it fix?

How to test?

What command should I have to run to test your PR?
What should I test through the UI?

Code review check list

projects/rero/ng-core/src/lib/record/search/aggregation/bucket-name.pipe.spec.ts

projects/rero/ng-core/src/lib/record/search/aggregation/bucket-name.pipe.ts

...s/rero/ng-core/src/lib/record/search/aggregation/list-filters/list-filters.component.spec.ts

projects/rero/ng-core/src/lib/record/search/aggregation/aggregation.component.ts

projects/rero/ng-core/src/lib/record/search/aggregation/bucket-name.pipe.ts

zannkukai · 2022-06-21T08:50:49Z

projects/rero/ng-core/src/lib/record/search/aggregation/buckets/buckets.component.ts

-    return (this.size === null || this.moreMode === false)
-      ? size
-      : this.size;
+    return this.size === null || this.moreMode === false ? size : this.size;


Are you sure about parentheses removal ?
COSMETIC : For readability, I prefer use ternary instruction on three separate line (my opinion)

zannkukai · 2022-06-21T08:53:07Z

projects/rero/ng-core/src/lib/record/search/aggregation/buckets/buckets.component.ts

-    return (this.size === null)
-      ? false
-      : this.buckets.length > this.size;
+    return this.size === null ? false : this.buckets.length > this.size;


COSMETIC : Seems more readable on three line ; especially when one of the option is itself a boolean comparaison.

zannkukai · 2022-06-21T08:59:22Z

...ects/rero/ng-core/src/lib/record/search/aggregation/list-filters/list-filters.component.html

+<ul *ngIf="filters?.length > 0" class="pl-0">
+    <li *ngFor="let filter of filters" class="list-unstyled mb-1">


OPINION : (should be discussed with all devs). Using styling in ng/core should be use only if really necessary (especially padding, margin, ...). Using them into ng/core will disallow to have custom styling in children project.

projects/rero/ng-core/src/lib/record/search/aggregation/list-filters/list-filters.component.ts

projects/rero/ng-core/src/lib/record/search/record-search.component.html

Garfield-fr · 2022-08-17T10:14:11Z

The commit message is too long (Max 50 chars).

Make a description for that.

And why you are 3 commits ?

zannkukai · 2022-08-17T11:19:02Z

projects/rero/ng-core/src/lib/record/search/aggregation/bucket-name.pipe.ts

+    // For language aggregation, we transform language code to human readable
+    // language.
+    if (aggregationKey === 'language') {
+      return this._translateLanguage.translate(bucket.key, this._translateService.currentLang);
+    }
+
+    // Simply translate the bucket key.
+    return this._translateService.instant(bucket.key);


cosmetic : a ternary instruction (I love them 😍) could be used

return (aggregationKey === 'language') ? this._translateLanguage.translate(bucket.key, this._translateService.currentLang) : this._translateService.instant(bucket.key)

zannkukai · 2022-08-17T11:23:22Z

...ects/rero/ng-core/src/lib/record/search/aggregation/list-filters/list-filters.component.html

@@ -0,0 +1,8 @@
+<ul *ngIf="filters?.length > 0" class="pl-0">
+    <li id="filter" *ngFor="let filter of filters" class="d-inline list-unstyled pr-2">


use an id attribute with a *ngFor should be avoid. An ID must be unique into the DOM. If we create multiple li, the same id cill be used.

Additionnally using a id attribute on this tag seems not required at all except for the spec.ts.
Two solutions :

either use an id='filters' attributes on the ul tag. Selectror changes to ul#filters li > button (to be tested)

either use an additional class (ex:'filter-value') on each li. Selectror changes to li.filter-value > button(to be tested)

zannkukai · 2022-08-17T11:29:50Z

...s/rero/ng-core/src/lib/record/search/aggregation/list-filters/list-filters.component.spec.ts

+  });
+
+  it('should display 2 buttons', () => {
+    const buttons = fixture.debugElement.nativeElement.querySelectorAll('li#filter > button');


See previous comment

zannkukai · 2022-08-17T11:31:10Z

projects/rero/ng-core/src/lib/record/search/aggregation/list-filters/list-filters.component.ts

+  /**
+   * All aggregations
+   */
+  @Input()
+  aggregations;


cosmetic : I prefer the condensed writing method (personal feeling)

/** All aggregations */ @Input() aggregations;

zannkukai · 2022-08-17T11:31:41Z

projects/rero/ng-core/src/lib/record/search/aggregation/list-filters/list-filters.component.ts

+   */
+  constructor(
+    private _recordSearchService: RecordSearchService,
+    private ref: ChangeDetectorRef


if private, then use the '_' convention --> _ref

zannkukai · 2022-08-17T11:33:41Z

projects/rero/ng-core/src/lib/record/search/record-search.component.ts

+    if (this._config.showFacetsIfNoResults) {
+      return false;
+    } else {
+      return !this.q && this.records.length === 0 && !this.showEmptySearchMessage;
+    }


possible ternary here.

* Removes the parent filters when a filter is selected in a hierarchical aggregation. * Removes useless methods. * The aggregationsExpand configuration can be a function. * Adds indeterminate state to the parent filters. * Stores the aggregationKey and parent properties in each bucket. Co-Authored-by: Johnny Mariéthoz <Johnny.Mariethoz@rero.ch>

Co-Authored-by: Johnny Mariéthoz Johnny.Mariethoz@rero.ch Co-Authored-by: Valeria Granata <valeria@chaw.com>

* correct Library facet in organisation view * improve Publication year facet Co-Authored-by: Valeria Granata <valeria@chaw.com>

The comments were resolved.

vgranata force-pushed the grv-facet-filters branch 2 times, most recently from 8af9113 to 226d638 Compare June 20, 2022 15:18

vgranata marked this pull request as ready for review June 20, 2022 15:25

vgranata requested review from zannkukai, jma and Garfield-fr June 20, 2022 15:27

This was referenced Jun 20, 2022

documents: open aggregation with filters and add more facets for documents search rero/rero-ils-ui#867

Merged

documents: add more facets for documents search rero/rero-ils#2953

Merged

Garfield-fr previously requested changes Jun 21, 2022

View reviewed changes

zannkukai reviewed Jun 21, 2022

View reviewed changes

vgranata force-pushed the grv-facet-filters branch from 226d638 to 283bacf Compare June 22, 2022 15:02

vgranata force-pushed the grv-facet-filters branch 3 times, most recently from d6b92c9 to 4ae14ef Compare July 25, 2022 14:30

vgranata force-pushed the grv-facet-filters branch from 4ae14ef to c62977a Compare August 17, 2022 07:14

vgranata requested review from Garfield-fr and zannkukai August 17, 2022 07:24

zannkukai requested changes Aug 17, 2022

View reviewed changes

vgranata force-pushed the grv-facet-filters branch from c62977a to 0d60c09 Compare August 18, 2022 15:07

zannkukai approved these changes Aug 19, 2022

View reviewed changes

jma approved these changes Aug 22, 2022

View reviewed changes

vgranata mentioned this pull request Aug 22, 2022

search: keep only the children filter #501

Closed

7 tasks

vgranata force-pushed the grv-facet-filters branch from 0d60c09 to 80d2a93 Compare August 22, 2022 15:49

jma and others added 2 commits August 22, 2022 17:53

search: add filters list

9f59d10

Co-Authored-by: Johnny Mariéthoz Johnny.Mariethoz@rero.ch Co-Authored-by: Valeria Granata <valeria@chaw.com>

vgranata force-pushed the grv-facet-filters branch 2 times, most recently from b84b8c3 to 14b172b Compare August 23, 2022 12:24

PascalRepond approved these changes Aug 23, 2022

View reviewed changes

search: hide searchFilters from filters list

e1bf4c1

* correct Library facet in organisation view * improve Publication year facet Co-Authored-by: Valeria Granata <valeria@chaw.com>

vgranata force-pushed the grv-facet-filters branch from 14b172b to e1bf4c1 Compare August 23, 2022 14:54

PascalRepond removed the request for review from Garfield-fr August 24, 2022 06:44

vgranata requested review from Garfield-fr and removed request for Garfield-fr August 24, 2022 08:39

vgranata merged commit 2befc12 into rero:staging Aug 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

search: keep only the children filter and add filter list #503

search: keep only the children filter and add filter list #503

vgranata commented Jun 20, 2022 •

edited

Loading

zannkukai Jun 21, 2022

zannkukai Jun 21, 2022

zannkukai Jun 21, 2022

Garfield-fr commented Aug 17, 2022 •

edited

Loading

zannkukai Aug 17, 2022

zannkukai Aug 17, 2022

zannkukai Aug 17, 2022

zannkukai Aug 17, 2022

zannkukai Aug 17, 2022

zannkukai Aug 17, 2022

		<ul *ngIf="filters?.length > 0" class="pl-0">
		<li *ngFor="let filter of filters" class="list-unstyled mb-1">

		@@ -0,0 +1,8 @@
		<ul *ngIf="filters?.length > 0" class="pl-0">
		<li id="filter" *ngFor="let filter of filters" class="d-inline list-unstyled pr-2">

search: keep only the children filter and add filter list #503

search: keep only the children filter and add filter list #503

Conversation

vgranata commented Jun 20, 2022 • edited Loading

Screenshot

Why are you opening this PR?

How to test?

Code review check list

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garfield-fr commented Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgranata commented Jun 20, 2022 •

edited

Loading

Garfield-fr commented Aug 17, 2022 •

edited

Loading