Fixed bug with filtering and pagination. #125
Conversation
dmitriy-borzenko
requested review from
EvilAlexei,
sergiimk and
zaychenko-sergei
| public get allHistory(): MetadataBlockFragment[] { | ||
| return Array.from(this.mapHistory.values()) | ||
| .map((item) => item.history) | ||
| .reduce((acc, cur) => [...acc, ...cur], []); |
There was a problem hiding this comment.
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.
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| @Input() public datasetHistory: MaybeNull<DatasetHistoryUpdate>; | ||
| @Input() public currentBlockHash: string; | ||
| @Input() public datasetInfo: DatasetInfo; | ||
| @Input() public allHistory: MetadataBlockFragment[]; |
There was a problem hiding this comment.
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.
| @Input() public allHistory: MetadataBlockFragment[]; | |
| @Input() public allHistory: MetadataBlockFragment[] = []; |
| 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 |
There was a problem hiding this comment.
-
I suggest to place
.page-item .page-linkinside 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
| 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 |
|
@dmitriy-borzenko Discussed this topic with @sergiimk today, and we decided:
|
No description provided.