Skip to content
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

UI: Minting success page #37

Merged
merged 10 commits into from
Mar 12, 2024
Merged

UI: Minting success page #37

merged 10 commits into from
Mar 12, 2024

Conversation

kpyszkowski
Copy link
Contributor

@kpyszkowski kpyszkowski commented Feb 15, 2024

Ref: #5

This PR introduces minting success page

image

Please note: the summary list (above the button) will be included with separate PR when required changes will get merged regarding to #36

Copy link
Contributor

@michalsmiarowski michalsmiarowski left a comment

Choose a reason for hiding this comment

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

Left some comments to look at before the merge

src/pages/tBTC/Bridge/components/DepositDetailsStep.tsx Outdated Show resolved Hide resolved
src/pages/tBTC/Bridge/DepositDetails.tsx Outdated Show resolved Hide resolved
src/pages/tBTC/Bridge/DepositDetails.tsx Outdated Show resolved Hide resolved
src/pages/tBTC/Bridge/DepositDetails.tsx Outdated Show resolved Hide resolved
src/pages/tBTC/Bridge/DepositDetails.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@michalsmiarowski michalsmiarowski left a comment

Choose a reason for hiding this comment

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

Left some non-blocking comments but overall looking good

Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking, but should be fixed (probably in a separate PR)

In the minting flow we have two different success steps

a) minting-completed" - when the optimistic minting is finalized but the tbtc tokens are not send to the address yet
b) completed - when the optimistic minting is finalized and the tbtc tokens are sent to the receiver

It looks like we distinguish that in the main page but we don`t show that in the timeline

image

Here, for complete step, the last item in the timeline should be completed but it is active in both cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this line should be updated?

id: DepositDetailsStep.MintingCompleted,

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, I think the issue is here:

const isComplete = itemIdIndex < currentStepIndex

itemIdIndex is never lower than currentStepIndex when it comes to last timeline item.

Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking, but should be addressed in a separate PR:

Timeline and Transaction history looks little bit off on smaller screens IMO. We should at least give them the same margins on the left and right as for the Token Contract button or center it.

image

@michalsmiarowski michalsmiarowski merged commit ce751e8 into main Mar 12, 2024
3 checks passed
@michalsmiarowski michalsmiarowski deleted the ui/minting-success-page branch March 12, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants