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: Initiate Minting step #22

Merged
merged 29 commits into from
Feb 21, 2024
Merged

UI: Initiate Minting step #22

merged 29 commits into from
Feb 21, 2024

Conversation

kpyszkowski
Copy link
Contributor

@kpyszkowski kpyszkowski commented Jan 9, 2024

Ref: #5
Depends on: #12

This pull request introduces redesigned Initiate Minting minting flow step.
localhost_3000_tBTC_mint(iPad Air) (6)

Additional changes:

  • Toast component with collapsible details, adjustable position and orientation:
    Position: center, expanded and collapsed details
    Position: left
    Position: right
  • LabeledBadge component

The `LabeledBadge` component will be reused in the future.
Additional changes: adjusted spacings.
Moved ModalHeader rendering to higher order component with additional
label parameter. Restyled Spinner component. Added onError callback when
deposit transaction is failed with storing error data (entry point for
Toast component rendering).
Copied from Treshold Dashboard
Restyled component, added variable orientation and position, added
optional collapsible details, improved responsive behavior.
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. It looks like lot of the code is copier from token-dashboard when we've already done a review. Some of the code was also cherry-picked in #21 where I've also done a review 😉

I would love to test it out once we merge other PRs

src/pages/tBTC/Bridge/Minting/InitiateMinting.tsx Outdated Show resolved Hide resolved
src/pages/tBTC/Bridge/Minting/InitiateMinting.tsx Outdated Show resolved Hide resolved
src/theme/Alert.ts Outdated Show resolved Hide resolved
@kpyszkowski kpyszkowski mentioned this pull request Feb 13, 2024
Copy link
Contributor Author

@kpyszkowski kpyszkowski left a comment

Choose a reason for hiding this comment

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

Addressed review comments

src/pages/tBTC/Bridge/Minting/InitiateMinting.tsx Outdated Show resolved Hide resolved
src/pages/tBTC/Bridge/Minting/InitiateMinting.tsx Outdated Show resolved Hide resolved
src/theme/Alert.ts Outdated Show resolved Hide resolved
This commit reverts changes lost after main branch merge.
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 more comments

src/components/TransactionDetails/index.tsx Outdated Show resolved Hide resolved
src/components/TransactionDetails/index.tsx Show resolved Hide resolved
src/components/TransactionDetails/index.tsx Outdated Show resolved Hide resolved
The `onPreviousStepClick` prop, if given, conditionally renders previous
step button.
It's incorrect to wrap general-purpose elements like `div` with
semantically meaningful elements like `p`. This commit resolves the
issue morphing `p` into `div`.
Refactored styles to be defined as chakra theme. Replaced boolean style
props with variant prop
@michalsmiarowski michalsmiarowski self-requested a review February 21, 2024 10:48
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.

LGTM

@michalsmiarowski michalsmiarowski merged commit 491e47a into main Feb 21, 2024
3 checks passed
@michalsmiarowski michalsmiarowski deleted the ui/initiate-minting-step branch February 21, 2024 10:50
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