Align contract event log l&f with transactions #2812
Conversation
jacogr
added
A3-inprogress ⏳
jacogr
added
A0-pleasereview 🤓
jacogr
changed the title
| const block = blocks[event.blockNumber.toString()]; | ||
| const transaction = transactions[event.transactionHash] || {}; | ||
| const classes = `${styles.event} ${styles[event.state]}`; | ||
| const url = `https://${isTest ? 'testnet.' : ''}etherscan.io/tx/${event.transactionHash}`; |
There was a problem hiding this comment.
Couldn't we have this URL constructor more DRY ? If etherscan ever goes down, or change its URL structure, it would be good to only modify it in one place whether than in every component
There was a problem hiding this comment.
I'm going to log these seperately - think they are valid points, just not quite up to the task at exactly this point in time.
There was a problem hiding this comment.
| return ( | ||
| <Container> | ||
| <ContainerTitle title='events' /> | ||
| <table className={ styles.events }> | ||
| <tbody>{ rows }</tbody> | ||
| <tbody>{ events.map((event) => <Event event={ event } key={ event.key } />) }</tbody> |
There was a problem hiding this comment.
Couldn't we pass the block and transaction info directly here, like that every Event Component won't have the whole blocks and transactions state Objects in their props? They can still be the ones triggering the fetching, but only receiving the right block and transaction Object. Not sure if it's a real perf. gain though...
There was a problem hiding this comment.
As these will be references, there won't be additional copies made. So at least from an overhead point of view things should be fine.
As a general point, you are probably right - no need to do connect on Events & Event if it can be helped.
|
Looks really nice in the UI! Quick remark: couldn't a progress loading bar be displayed while fetching the events? |
ngotchac
added
A6-mustntgrumble 💦
|
I'm actually worried about the event/transactions/blocks - at least to main untrained eye it seems much slower now with Redux. In addition we are now keeping state we never get rid of, may not be too much of a concern in "normal use". |
|
That's true, but it seems safer and removes a lot of errors we had before when switching views while fetching content, then setting state on unmounted components. |
Closes #2807