tBTC minting redesign - Phase I - part 1/4 #651
Conversation
Rebuilt and restyled component according to latest design updates
Added checkbox, implemented download logic and disabled modal
Implemented useToast hook with spawner function and root element, hook supports multiple instances handled by Redux store, implemented more performant setTimeout alternative utility function using requestAnimationFrame API
Implemented component, added RangeOperatorType type definition, added utility functions: getRangeSign, getDurationByAmount and getNumberOfConfirmationsAmount. Placed component into MakeDeposit step.
Removed redundant UI, fixed toast Icon alignment, added Step heading Badge, updated Timeline heading
Added appropriate toast message when deposit is received, implemented toasts identifier generation and introduced removeToasts method, renamed method: toast -> addToast, improved toasts positioning
Refactored useToast hook: divided structure into separate components, improved typings
Implemented DurationWidget component, applied minor UI improvements for MintingTimeline and BridgeLayoutAsideSection components
Reorganized types related with DurationTiers and DurationWidget components, removed unused import.
Redesigned Step 3: integrated modal contents into step, disabled modal, removed unused imports. NOTE: Transaction details will be updated with Phase II. It's corelated with Solana integration. (?)
Since current Chakra version uses deprecated version of animation library, mount/unmount animations seems to glitch between rerenders. I couldn't fix it so I revert changes for now.
|
Preview uploaded to https://preview.dashboard.test.threshold.network/tbtc-minting-redesign/index.html. |
1 similar comment
|
Preview uploaded to https://preview.dashboard.test.threshold.network/tbtc-minting-redesign/index.html. |
|
@michalsmiarowski can we get a review here? |
|
Preview uploaded to https://preview.dashboard.test.threshold.network/tbtc-minting-redesign/index.html. |
| <DurationTiers | ||
| mt="6" | ||
| items={[ | ||
| { | ||
| amount: 0.1, | ||
| rangeOperator: "less", | ||
| currency: "BTC", | ||
| }, | ||
| { | ||
| amount: 1, | ||
| rangeOperator: "less", | ||
| currency: "BTC", | ||
| }, | ||
| { | ||
| amount: 1, | ||
| rangeOperator: "greaterOrEqual", | ||
| currency: "BTC", | ||
| }, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
I don't think this is a good way to pass the items to the DurationTiers. The way you wrote the component is that it displays the duration times for confirmations for bitcoin transactions - you put the logic for that inside the component, which in result makes the component less reusable. This is not bad, because not every component have to be super reusable, but I believe we should pass less arguments to the component and let the component handle the rest of the logic.
Instead of the amount I would pass the txConfirmationNumbers and remove rangeOperator and currency. This is something that should be handled in the component, probably in some kind of a helper function that will return the value witt the range operator based on the txConfirmationNumber passed. Inside that helper function we would call getDuraiontByNumberOfConfimrations (see my comment here: #651 (comment)) and based on the minutes returned from there we would print a string that we want to display. We could then use that logic in DurationWidget too.
I think it would also look better if we've just change it to singular tier component instead of making it rendering all of them by passing the data as items property. So instead of doing this:
<DurationTiers
mt="6"
items={[
{
transactionConfirmations: 1
},
{
transactionConfirmations: 3
},
{
transactionConfirmations: 6
},
]}
/>
or this:
<DurationTiers
mt="6"
transactionConfirmations={[1, 3, 6]}
/>
I would just extract the <Stack> and render it separately:
<Stack direction="row" spacing="6" mt="6">
<DurationTier transactionConfirmations={1} />
<DurationTier transactionConfirmations={3} />
<DurationTier transactionConfirmations={6} />
</Stack>
There was a problem hiding this comment.
If you name component the way you requested (prefixed by 'Mint') you indicate that the component is limited to be used in correlation with minting only.
That's a good point to remove the confirmation calculations from the component itself and keep it in presentiontal role only but that way I would revert the name change. Components are reusable by definition, non-reusable component is not a component at all.
About component API - array of objects with strongly typed structure is not a dealbreaker, especially if you are using TypeScript. It's crucial to find a proper balance between reusability and overengineering. The problem is when props are eg. polymorphic or deeply nested with unspecified structure which can lead to misunderstandings when consuming component. With type safety and documentation (which I should add :D) it's not a big deal. TypeScript gives much more opportunities, so why don't use it? If it was JavaScript I wouldn't go for it.
My suggestion is:
- Revert name change of components.
- Remove duration/confirmation calculations from components so its roles will be strictly presentational (it gets data, and displays data).
- Calculate duration and/or confirmations one step before the render of component and pass it as props as you suggested.
- Add JSDoc documentation for component.
Added documentation for functions
|
Preview uploaded to https://preview.dashboard.test.threshold.network/tbtc-minting-redesign/index.html. |
1 similar comment
|
Preview uploaded to https://preview.dashboard.test.threshold.network/tbtc-minting-redesign/index.html. |
|
Preview uploaded to https://preview.dashboard.test.threshold.network/tbtc-minting-redesign/index.html. |
|
Preview uploaded to https://preview.dashboard.test.threshold.network/tbtc-minting-redesign/index.html. |
|
Preview uploaded to https://preview.dashboard.test.threshold.network/tbtc-minting-redesign/index.html. |
1 similar comment
|
Preview uploaded to https://preview.dashboard.test.threshold.network/tbtc-minting-redesign/index.html. |
|
Preview uploaded to https://preview.dashboard.test.threshold.network/tbtc-minting-redesign/index.html. |
Improve `mintingStep` state change when deposit is resumed.
kpyszkowski
force-pushed
the
tbtc-minting-redesign
branch
from
1b6e033 to
507c500
Compare
|
Preview uploaded to https://preview.dashboard.test.threshold.network/tbtc-minting-redesign/index.html. |
|
Preview uploaded to https://preview.dashboard.test.threshold.network/tbtc-minting-redesign/index.html. |
|
Preview uploaded to https://preview.dashboard.test.threshold.network/tbtc-minting-redesign/index.html. |
Ref: #590
I've accomplished the first part of ongoing changes in Phase I. The current progress status and PR scope are presented in this objectives checklist.
I've also left some comments on Figma to inform and explain about the unapplied changes I will implement in future parts/phases.