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

Added VaultLiquidationTransactionTable and VaultLiquidationTransactionFlow #454

Merged
merged 19 commits into from
Sep 15, 2022

Conversation

aomafarag
Copy link
Contributor

@aomafarag aomafarag commented Sep 9, 2022

Closes #437.

Screenshot from 2022-09-09 18-20-27

Checklist:

  • issue number linked above after pound (#)
    • replace "Closes " with "Contributes to" or other if this PR does not close the issue
  • issue checkboxes are all addressed
  • manually checked my feature / not applicable
  • wrote tests / not applicable
  • attached screenshots / not applicable

Missing

  • Add AnimatedArrow after merging
  • Add Explain texts

@aomafarag
Copy link
Contributor Author

@valiafetisov, isn't there a description for the transaction fees already in the WhatIsCatch component?

@valiafetisov
Copy link
Contributor

You can remove explainer over the combined transaction fees if you're asking about it since there is just one transaction. I would also call it Transaction fee

@zoey-kaiser zoey-kaiser changed the title Add VaultLiquidationTransactionTable component and stories Added VaultLiquidationTransactionTable and VaultLiquidationTransactionFlow Sep 14, 2022
@valiafetisov
Copy link
Contributor

It's of course better for the user to not suddenly hide anything, but in this case I think it's a good trade-off between UX and DX. In case vault is liquidated, I think it's fine to just hide everything in the page except for the title, close sign and the alert specifying what happened. The added value of the table that doesn't show anything and panels that doesn't work is very low.

frontend/components/common/other/AnimatedArrow.vue Outdated Show resolved Hide resolved
frontend/components/common/other/AnimatedArrow.vue Outdated Show resolved Hide resolved
Comment on lines +142 to +144
wasLiquidated(): boolean {
return this.vaultTransaction.state === 'liquidated';
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure why types allow this check, since VaultTransactionNotLiquidated doesn't have this kind of state. Types doesn't work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Types in my IDE never worked, but I am surprised it was not caught in the CI. However the comment above should fix it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Types in my IDE never worked

OH WOW! Why was it never reported? Or did I miss it?

Comment on lines +99 to +106
vaultTransaction: {
type: Object as Vue.PropType<VaultTransaction>,
required: true,
},
liquidationLimits: {
type: Object as Vue.PropType<LiquidationLimits>,
required: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

The liquidation limits are actually part of the vaultTransaction, but I guess you will fix it when you will put together the container 🙂

@valiafetisov valiafetisov merged commit 362f42d into main Sep 15, 2022
@valiafetisov valiafetisov deleted the vault-transaction-table branch September 15, 2022 16:55
@valiafetisov
Copy link
Contributor

The other thing that will need to be updated soon the liquidated vault state: #460 (comment)

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.

Create VaultLiquidationTransactionTable component
3 participants