Skip to content

Fixed bug with filtering and pagination. #125

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions src/app/components/pagination-component/pagination-component.sass
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,18 @@
display: grid
justify-content: center

ngb-pagination ::ng-deep .page-item .page-link
border: none !important
display: flex
justify-content: center
align-items: center
border-radius: 6px
ngb-pagination
&::ng-deep .page-item .page-link
border: none !important
display: flex
justify-content: center
align-items: center
border-radius: 6px
& span
display: none
ngb-pagination ::ng-deep *
font-size: 14px
ngb-pagination
&::ng-deep *
font-size: 14px
Comment on lines +9 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

  • I suggest to place .page-item .page-link inside the &::ng-deep, in cease there will be other styles for pagination.

  • Also, yould you please remove parent & here & span? It's not needed

  • Could you please remove !important?
    I think it should work without it.

importatn is kind of last resort, and should be avoided in most of the cases,
usually it could be handled by adding "weight" to selectors - add class/id, or parent, or additional selector in a link

  • This looks wrong, It will generate wrong styles, and I think font-size could be placed on the .page-link, I assume there will be no other text/inline elements
    ngb-pagination
      &::ng-deep *
        font-size: 14px
Suggested change
ngb-pagination
&::ng-deep .page-item .page-link
border: none !important
display: flex
justify-content: center
align-items: center
border-radius: 6px
& span
display: none
ngb-pagination ::ng-deep *
font-size: 14px
ngb-pagination
&::ng-deep *
font-size: 14px
ngb-pagination
&::ng-deep
.page-item .page-link
border: none
display: flex
justify-content: center
align-items: center
border-radius: 6px
font-size: 14px
span
display: none

.page_button
display: inline-block
width: 16px
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
[collectionSize]="(pageInfo.totalPages || currentPage) * 10"
[maxSize]="7"
[(page)]="currentPage"
[directionLinks]="false"
[rotate]="true"
[ellipses]="true"
(pageChange)="onPageChange(currentPage)"
aria-label="Custom pagination"
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@
</div>
<div class="d-flex flex-column gap-4" *ngIf="datasetHistory">
<ng-container
*ngIf="datasetHistory.history | eventTypeFilter: eventTypeFilters | blockHashFilter: searchHash as result"
*ngIf="
datasetHistory.history
| eventTypeFilter: eventTypeFilters:allHistory
| blockHashFilter: searchHash:allHistory as result
"
>
<div *ngFor="let block of result" class="d-flex lh-lg align-items-center justify-content-between">
<div class="flex-grow-1 no-wrap lh-1">
Expand All @@ -52,12 +56,16 @@
<ng-container *ngIf="result.length === 0">
<p class="mt-4 text-center">No block matches</p>
</ng-container>
<app-pagination
*ngIf="result.length"
[currentPage]="currentPage - 1"
[pageInfo]="datasetHistory.pageInfo"
(pageChangeEvent)="onPageChange($event)"
></app-pagination>
<ngb-pagination
class="d-flex justify-content-center mt-4"
(pageChange)="onPageChange($event)"
[collectionSize]="eventTypeFilters.length || searchHash ? result.length : allHistory.length"
[directionLinks]="false"
[maxSize]="7"
[page]="1"
[rotate]="true"
[ellipses]="true"
></ngb-pagination>
</ng-container>
</div>
</div>
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { ChangeDetectionStrategy, Component, EventEmitter, Input, Output } from
import { DatasetInfo } from "src/app/interface/navigation.interface";
import { MaybeNull } from "src/app/common/app.types";
import { IDropdownSettings } from "ng-multiselect-dropdown";
import { MetadataBlockFragment } from "src/app/api/kamu.graphql.interface";

@Component({
selector: "app-block-navigation",
Expand All @@ -15,6 +16,7 @@ export class BlockNavigationComponent {
@Input() public datasetHistory: MaybeNull<DatasetHistoryUpdate>;
@Input() public currentBlockHash: string;
@Input() public datasetInfo: DatasetInfo;
@Input() public allHistory: MetadataBlockFragment[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to initialize this parameter with an empty array, it will help avoid error if for some reason the parameter will be undefined or null.

Suggested change
@Input() public allHistory: MetadataBlockFragment[];
@Input() public allHistory: MetadataBlockFragment[] = [];

@Output() public onPageChangeEmit = new EventEmitter<number>();
public searchHash = "";
public currentPage = 1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,12 @@ import { MetadataBlockFragment } from "src/app/api/kamu.graphql.interface";
name: "blockHashFilter",
})
export class BlockHashFilterPipe implements PipeTransform {
transform(blocks: MetadataBlockFragment[], filter: string): MetadataBlockFragment[] {
transform(
blocks: MetadataBlockFragment[],
filter: string,
allHistory: MetadataBlockFragment[],
): MetadataBlockFragment[] {
if (filter === "") return blocks;
return blocks.filter((block: MetadataBlockFragment) => (block.blockHash as string).includes(filter));
return allHistory.filter((block: MetadataBlockFragment) => (block.blockHash as string).includes(filter));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,17 @@ import { MetadataBlockFragment } from "src/app/api/kamu.graphql.interface";
name: "eventTypeFilter",
})
export class EventTypeFilterPipe implements PipeTransform {
transform(blocks: MetadataBlockFragment[], filters: string[]): MetadataBlockFragment[] {
transform(
blocks: MetadataBlockFragment[],
filters: string[],
allHistory: MetadataBlockFragment[],
): MetadataBlockFragment[] {
if (!filters.length) {
return blocks;
} else {
const result = [] as MetadataBlockFragment[];
filters.forEach((filter: string) =>
result.push(...blocks.filter((block) => block.event.__typename === filter)),
result.push(...allHistory.filter((block) => block.event.__typename === filter)),
);
return result;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
<div class="container-content">
<app-block-navigation
[datasetHistory]="datasetHistoryUpdate"
[allHistory]="allHistory"
[currentBlockHash]="blockHash"
[datasetInfo]="datasetInfo"
(onPageChangeEmit)="onPageChange($event)"
Expand Down
23 changes: 21 additions & 2 deletions src/app/dataset-block/metadata-block/metadata-block.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { NavigationEnd, Router } from "@angular/router";
import { BlockService } from "./block.service";
import { AppDatasetSubscriptionsService } from "src/app/dataset-view/dataset.subscriptions.service";
import _ from "lodash";
import { MetadataBlockFragment } from "src/app/api/kamu.graphql.interface";

@Component({
selector: "app-metadata-block",
Expand All @@ -37,6 +38,7 @@ export class MetadataBlockComponent extends BaseProcessingComponent implements O
public blockHash: string;
public datasetHistoryUpdate: MaybeNull<DatasetHistoryUpdate> = null;
private blocksPerPage = 10;
public mapHistory = new Map<number, DatasetHistoryUpdate>();

ngOnInit(): void {
this.datasetInfo = this.getDatasetInfoFromUrl();
Expand All @@ -54,14 +56,31 @@ export class MetadataBlockComponent extends BaseProcessingComponent implements O
this.loadMetadataBlock(),
this.loadHistory(),
this.appDatasetSubsService.onDatasetHistoryChanges.subscribe((result: DatasetHistoryUpdate) => {
this.datasetHistoryUpdate = result;
this.mapHistory.set(result.pageInfo.currentPage, result);
if (result.pageInfo.hasNextPage && result.pageInfo.totalPages !== this.mapHistory.size) {
this.loadHistory(result.pageInfo.currentPage + 1);
}
const firstPageHistory = this.mapHistory.get(0);
if (firstPageHistory) {
this.datasetHistoryUpdate = firstPageHistory;
}
Comment on lines +59 to +66
Copy link
Contributor

Choose a reason for hiding this comment

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

Try implementing a lazy load algorithm. It should request and cache the next page only if really needed, if the consumer keeps asking for more data.

Imagine you have a history of a few hundred pages, and the answer to the filter, like 10 events of certain type, is satisfied by the time you processed page 3 (let's say you've found enough nodes to show a page of filtered results), It would be unnecessary to continue loading history. But maybe user requests another 10 events of this type, so you would continue loading to page 8 to satisfy it. Loading hundreds of pages still does not look reasonable.

this.cdr.detectChanges();
}),
);
}

public get allHistory(): MetadataBlockFragment[] {
return Array.from(this.mapHistory.values())
.map((item) => item.history)
.reduce((acc, cur) => [...acc, ...cur], []);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks N^2 to me by complexity - you are creating lists and copying all elements + another page's elements on each step.

Consider implementing something like a Composite Iterator.
Or maybe a wrapper interface around all arrays without copying them and access to a node by index, which would map to one of the arrays depending on index / pageSize.

}

public onPageChange(currentPage: number): void {
this.trackSubscription(this.loadHistory(currentPage - 1));
const currentHistory = this.mapHistory.get(currentPage - 1);
if (currentHistory) {
this.datasetHistoryUpdate = currentHistory;
this.cdr.detectChanges();
}
}

private loadMetadataBlock(): Subscription {
Expand Down
2 changes: 2 additions & 0 deletions src/app/dataset-block/metadata-block/metadata-block.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import { EventTimePropertyComponent } from "./components/event-details/component
import { OrderPropertyComponent } from "./components/event-details/components/common/order-property/order-property.component";
import { CommandPropertyComponent } from "./components/event-details/components/common/command-property/command-property.component";
import { StepTypePropertyComponent } from "./components/event-details/components/common/step-type-property/step-type-property.component";
import { NgbPaginationModule } from "@ng-bootstrap/ng-bootstrap";

@NgModule({
declarations: [
Expand Down Expand Up @@ -98,6 +99,7 @@ import { StepTypePropertyComponent } from "./components/event-details/components
DisplayTimeModule,
DisplayHashModule,
SharedModule,
NgbPaginationModule,
],
exports: [MetadataBlockComponent],
})
Expand Down
22 changes: 11 additions & 11 deletions src/styles.sass
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ a
& .navbar
padding: 0
[data-test-id="appLogo"]
background: none
border: none
background: none
border: none
.mat-progress-spinner circle
stroke: $app-darkSeaGreen !important

Expand Down Expand Up @@ -296,7 +296,7 @@ a:visited
a:visited
color: inherit
router-outlet
display: none
display: none
.app-content__card-columns
column-count: 1
grid-column-gap: 1.25rem
Expand Down Expand Up @@ -499,9 +499,9 @@ router-outlet
> i
font-size: 18px
[data-test-id="searchInput"]
padding-left: 35px
&::placeholder
color: $app-white
padding-left: 35px
&::placeholder
color: $app-white

#searchInput.searchSidenav .input-group > [data-test-id="searchInput"].form-control
background: $app-white
Expand Down Expand Up @@ -883,13 +883,13 @@ app-account .dropdown-menu
width: 20px
font-size: 20px
[role="menu"] .UnderlineNav-item
display: flex
justify-content: flex-start
align-items: center
display: flex
justify-content: flex-start
align-items: center
details summary::-webkit-details-marker,
details summary::marker
display: none
content: ""
display: none
content: ""
.UnderlineNav-item
padding: 8px 16px
font-size: 14px
Expand Down