web-ui: added color effect on newest blocks (fixes #20) #25
Conversation
| this.blocks = response; | ||
| } | ||
|
|
||
| private countNewestBlocks(newBlocks: Block[]) { |
There was a problem hiding this comment.
I would avoid functions with side effects as much as possible, as a example, you mutating the totalNewestBlocks, instead you can return the number of new blocks and do whatever you want with the number, think about which version would be simpler to test.
| return; | ||
| } | ||
|
|
||
| const lastestBlockHash: string = this.latestBlock.hash; |
There was a problem hiding this comment.
I would just pass this as an argument, as a hint, the height attribute from a block is just the block number and it is unique, it should be simpler to use that instead of the block hash, again, think on simplicity to test.
| const lastestBlockHash: string = this.latestBlock.hash; | ||
| let totalNewBlocks = 0; | ||
|
|
||
| for (let i = 0; i < newBlocks.length; i++) { |
There was a problem hiding this comment.
A loop is error prone, use collection transformation functions instead, like map and filter, if you use the latest block height, it is simple to filter by the blocks having a higher height.
| @@ -92,4 +115,8 @@ export class LatestBlocksComponent implements OnInit, OnDestroy { | |||
| age(block: Block): string { | |||
| return ''; | |||
| } | |||
|
|
|||
| isBlockRecent(index: number): boolean { | |||
There was a problem hiding this comment.
In my opinion, checking this by an index is error prone, angular allows you to pass the block object as an argument to the function, on the testability side, it would be simpler to receive both arguments instead of referencing a mutable variable.
| @@ -70,9 +72,30 @@ export class LatestBlocksComponent implements OnInit, OnDestroy { | |||
| } | |||
|
|
|||
| private onBlockRetrieved(response: Block[]) { | |||
| this.countNewestBlocks(response); | |||
There was a problem hiding this comment.
What I would do is to get the list of new blocks and store them in a mutable variable, something like this.newBlocks = this.getNewBlocks(this.blocks || [], response) with a function like private getNewBlocks(currentBlocks: Block[], latestBlocks: Block[]): Block[].
This might give the effect that the initial load will paint all blocks which should be acceptable.
| @@ -70,9 +72,30 @@ export class LatestBlocksComponent implements OnInit, OnDestroy { | |||
| } | |||
|
|
|||
| private onBlockRetrieved(response: Block[]) { | |||
| this.countNewestBlocks(response); | |||
| this.latestBlock = response[0]; | |||
There was a problem hiding this comment.
This should not be required at all.
| @@ -26,6 +26,8 @@ import { ErrorService } from '../../services/error.service'; | |||
| export class LatestBlocksComponent implements OnInit, OnDestroy { | |||
|
|
|||
| blocks: Block[]; | |||
| private totalNewestBlocks: number; | |||
There was a problem hiding this comment.
I would define private newBlocks: Block[]; instead.
| @@ -26,6 +26,8 @@ import { ErrorService } from '../../services/error.service'; | |||
| export class LatestBlocksComponent implements OnInit, OnDestroy { | |||
|
|
|||
| blocks: Block[]; | |||
| private totalNewestBlocks: number; | |||
| private latestBlock: Block; | |||
There was a problem hiding this comment.
This should not be required at all.
| @@ -26,7 +26,7 @@ <h4>{{'message.loadingLatestBlocks' | translate}}</h4> | |||
| </thead> | |||
|
|
|||
| <tbody> | |||
| <tr *ngFor="let item of blocks"> | |||
| <tr *ngFor="let item of blocks; let i = index" [ngClass]="{ 'latest-block': isBlockRecent(i) }"> | |||
There was a problem hiding this comment.
The index is not required, you can just call isNewBlock(item) where that function returns true if the item hash or height exists in the newBlocks variable, this can be done easily by the filtering over it.
| } | ||
|
|
||
| this.totalNewestBlocks = totalNewBlocks; | ||
| return newBlocks.filter(newBlock => newBlock.height > this.blocks[0].height); |
There was a problem hiding this comment.
you are assuming that the blocks are in certain order, while today that's true, we don't know if it would be tomorrow. you can easily get the current latest block height and compare against that.
| this.blocks = response; | ||
| } | ||
|
|
||
| private countNewestBlocks(newBlocks: Block[]) { | ||
| private getNewBlocks(newBlocks: Block[]): Block[] { | ||
| if (!this.blocks) { |
There was a problem hiding this comment.
again, this is harder to test than just passing the required argument to the function, please stop using global state.
| this.blocks = response; | ||
| } | ||
|
|
||
| private countNewestBlocks(newBlocks: Block[]) { | ||
| private getNewBlocks(newBlocks: Block[]): Block[] { | ||
| if (!this.blocks) { |
There was a problem hiding this comment.
in fact, if you receive the latest block height as an argument, this conditional will be useless.
| @@ -26,6 +26,7 @@ import { ErrorService } from '../../services/error.service'; | |||
| export class LatestBlocksComponent implements OnInit, OnDestroy { | |||
|
|
|||
| blocks: Block[]; | |||
| private latestBlockHeight: number; | |||
There was a problem hiding this comment.
please, initialize this to 0 or -1 to avoid undefined issues.
| @@ -70,6 +71,7 @@ export class LatestBlocksComponent implements OnInit, OnDestroy { | |||
| } | |||
|
|
|||
| private onBlockRetrieved(response: Block[]) { | |||
| this.latestBlockHeight = this.blocks ? this.blocks[0].height : 0; | |||
There was a problem hiding this comment.
please just get the maximum instead.
- A message is displayed when the transaction list is not available. - The transaction list is loaded after the block data. - When the paginated transactions aren't available, the transaction list from the block data is displayed.
Problem
On latest block view, the newest blocks were hard to notice
Solution
Added a color effect on newest blocks
Result