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

177 only show transfers for connected account #228

Merged
merged 9 commits into from
Aug 11, 2023

Conversation

gonzamontiel
Copy link
Contributor

Closes #177

@gonzamontiel gonzamontiel linked an issue Aug 8, 2023 that may be closed by this pull request
@netlify
Copy link

netlify bot commented Aug 8, 2023

Deploy Preview for rococo-souffle-a625f5 ready!

Name Link
🔨 Latest commit 7545219
🔍 Latest deploy log https://app.netlify.com/sites/rococo-souffle-a625f5/deploys/64d4bbd0c212c700082ffa88
😎 Deploy Preview https://deploy-preview-228--rococo-souffle-a625f5.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@gonzamontiel gonzamontiel requested a review from ebma August 8, 2023 07:50
src/helpers/spacewalk.ts Outdated Show resolved Hide resolved
src/pages/bridge/Transfers.tsx Outdated Show resolved Hide resolved
src/pages/bridge/Transfers.tsx Outdated Show resolved Hide resolved
@gonzamontiel gonzamontiel requested a review from ebma August 8, 2023 15:18
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

The code to solve the original issue ticket looks good now.

But the changes for the redeem dialog are wrong. If it timed out and is still pending, then we cannot just say it's cancelled. Because it's not. The user now can take actions for retrying or reimbursing. Instead it should show the following dialog, that I took from the figma.
image

Thus, I would like these changes to be removed, we should create a follow-up ticket to show proper pending redeem dialogs @prayagd.

@gonzamontiel
Copy link
Contributor Author

The code to solve the original issue ticket looks good now.

But the changes for the redeem dialog are wrong. If it timed out and is still pending, then we cannot just say it's cancelled. Because it's not. The user now can take actions for retrying or reimbursing. Instead it should show the following dialog, that I took from the figma. image

Thus, I would like these changes to be removed, we should create a follow-up ticket to show proper pending redeem dialogs @prayagd.

Oh, my bad, the status for timedout Back to Stellar transactions is 'Failed', I fixed it, and now it shows the correct dialog (at least according to the old designs). We still need a follow up ticket to implement the buttons and expected behavior, thanks for bringing that up.

@gonzamontiel gonzamontiel requested a review from ebma August 9, 2023 14:52
…sfers-for-connected-account

# Conflicts:
#	src/pages/bridge/TransferDialog.tsx
@prayagd
Copy link
Collaborator

prayagd commented Aug 10, 2023

@ebma @gonzamontiel Yes there would be tickets for updating the dailog boxes.

Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

Maybe showing a message that you have to connect to a wallet account first in order to see transfers, and a message that tells you you don't have any transfers so far (once connected to an account) would make sense @prayagd.

But this is not mentioned in the issue ticket, so the implemented changes LGTM

@gonzamontiel gonzamontiel merged commit 25ccf91 into staging Aug 11, 2023
5 checks passed
@gonzamontiel gonzamontiel deleted the 177-only-show-transfers-for-connected-account branch August 11, 2023 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only show transfers for connected account
3 participants