feat: cardano modal with governance #16003

vladimirvolek · 2024-12-17T11:45:42Z

Adding a new Cardano withdrawal modal.

packages/suite/src/components/suite/modals/ReduxModal/CardanoWithdrawModal.tsx

packages/suite/src/hooks/wallet/useCardanoStaking.ts

packages/suite/src/support/messages.ts

tomasklim

Please improve commit texts

We do not use
fix: modal heading
fix: text

Maybe just squash it all into one or better separate it into some logical parts. Thanks!

packages/suite/src/components/suite/modals/ReduxModal/CardanoWithdrawModal.tsx

packages/suite/src/hooks/wallet/useCardanoStaking.ts

packages/suite/src/components/suite/modals/ReduxModal/CardanoWithdrawModal.tsx

enjojoy · 2025-01-02T13:10:40Z

@vladimirvolek Please use NewModal component with its props instead of custom styling

If you're not sure about what components are available and how to use them you can run storybook locally
yarn nx run @trezor/components:storybook
And see what's available

packages/suite/src/components/suite/modals/ReduxModal/CardanoWithdrawModal.tsx

enjojoy · 2025-01-02T13:35:22Z

Also I see that conventional commits check failed

We use it like that, for example:

fix(suite): desc of the change
https://www.conventionalcommits.org/en/v1.0.0/

packages/suite/src/components/suite/modals/ReduxModal/CardanoWithdrawModal.tsx

vladimirvolek · 2025-01-03T01:10:43Z

Next round please? ✨

enjojoy · 2025-01-03T07:56:38Z

I think you shouldn't pass that big piece of text as a description, it meant to be rather a one sentence summary of the modal.

As you can see it moves the close button down and it looks misaligned (1st pic)

Please pass this text as a child so it's aligned better (2nd pic). If you want to make the text secondary you can wrap it into Text component.

<Text variant="tertiary">
    <Translation id="TR_CARDANO_WITHDRAW_MODAL_TITLE_DESCRIPTION" />
</Text>

mroz22 · 2025-01-03T08:05:55Z

suite-common/wallet-types/src/account.ts

+                  drep: {
+                      drep_id: string;
+                      hex: string;
+                      amount: string;
+                      active: boolean;
+                      active_epoch: number | null;
+                      has_script: boolean;
+                  } | null;


🤔 this field is not optional. doesn't it require db migration?

I might not be seeing the full picture so I'd like to ask what the staking object was added to this type here https://github.com/trezor/trezor-suite/pull/16003/files#diff-590423282c67d295c7292f90e100f2863ea22b056e49ae02f135a5840475fb9bR172 (actually full misc object). was it a bug? how come anybody did not miss it there before?

Yes it was missing.

vladimirvolek · 2025-01-03T14:39:06Z

Anything else to improve or fix?

mroz22 · 2025-01-03T15:09:16Z

should there be more changes to this branch, please stop using force pushes for each of them. It makes the review slightly less enjoyable. Also please leave resolving of comments to their authors.

packages/suite/src/components/suite/modals/ReduxModal/CardanoWithdrawModal.tsx

vladimirvolek · 2025-01-05T00:49:22Z

@adamhavel 8b5cd37

adamhavel · 2025-01-05T13:14:59Z

8b5cd37

Thanks! One last thing, use NewModal.Button instead of Button in the bottomContent

vladimirvolek · 2025-01-05T15:30:34Z

8b5cd37

Thanks! One last thing, use NewModal.Button instead of Button in the bottomContent

cdead7b

mroz22 · 2025-01-06T09:39:54Z

not sure what about other guys think but I'd say that there should not be any "fix" commits for previous commits in the same PR. fix commit should be reserved for fixing anything that is already in develop.

doing it like this doesn't really help when writing changelogs - I don't want to state that I am fixing something which in fact hadn't been broken before this PR.

edit: lets please go with fix-ups for the purpose of review. they should be squashed later ofc.

vladimirvolek · 2025-01-07T09:45:35Z

not sure what about other guys think but I'd say that there should not be any "fix" commits for previous commits in the same PR. fix commit should be reserved for fixing anything that is already in develop.
doing it like this doesn't really help when writing changelogs - I don't want to state that I am fixing something which in fact hadn't been broken before this PR.
edit: lets please go with fix-ups for the purpose of review. they should be squashed later ofc.

The review is done, so I can squash this into one commit, right?

tomasklim · 2025-01-07T09:52:48Z

@vladimirvolek

You can squash it but better is to separate it into some logical parts. Thanks!

e.g. blockchain-link changes in separate commit

fix: modal heading fix: text Update packages/suite/src/components/suite/modals/ReduxModal/CardanoWithdrawModal.tsx Co-authored-by: Marek Mahut <[email protected]> review fix: new modal fix: modal fix: migration fix(suite): Cardano withdraw modal UI fix(suite): Cardano withdraw modal UI - buttons

tomasklim · 2025-01-07T13:09:01Z

Closing in favor of #16228 to run CI and separate commits

vladimirvolek requested review from MiroslavProchazka and marekrjpolak as code owners December 17, 2024 11:45

jirih-stsh requested a review from tomasklim December 17, 2024 15:36

vladimirvolek changed the title ~~feat: cardano review modal with governance~~ feat: cardano modal with governance Dec 17, 2024

mmahut reviewed Dec 18, 2024

View reviewed changes

packages/suite/src/components/suite/modals/ReduxModal/CardanoWithdrawModal.tsx Outdated Show resolved Hide resolved

tomasklim reviewed Dec 30, 2024

View reviewed changes

packages/suite/src/hooks/wallet/useCardanoStaking.ts Outdated Show resolved Hide resolved

tomasklim reviewed Dec 30, 2024

View reviewed changes

packages/suite/src/support/messages.ts Outdated Show resolved Hide resolved

tomasklim requested changes Dec 30, 2024

View reviewed changes

tomasklim added the cardano Related to Cardano (ADA) blockchain platform label Dec 30, 2024

tomasklim requested review from jvaclavik and adamhavel December 30, 2024 16:24

tomasklim assigned jvaclavik and adamhavel Dec 30, 2024

mroz22 reviewed Jan 1, 2025

View reviewed changes

enjojoy requested changes Jan 2, 2025

View reviewed changes

adderpositive reviewed Jan 2, 2025

View reviewed changes

packages/suite/src/components/suite/modals/ReduxModal/CardanoWithdrawModal.tsx Outdated Show resolved Hide resolved

vladimirvolek force-pushed the trezor-withdraw-modal branch from 135321b to 2a0724b Compare January 3, 2025 00:58

mroz22 reviewed Jan 3, 2025

View reviewed changes

vladimirvolek force-pushed the trezor-withdraw-modal branch 3 times, most recently from b2b1418 to 05ac752 Compare January 3, 2025 14:33

adamhavel requested changes Jan 3, 2025

View reviewed changes

tomasklim unassigned jvaclavik and adamhavel Jan 4, 2025

tomasklim mentioned this pull request Jan 5, 2025

test CI - feat: cardano review modal with governance #16172

Closed

vladimirvolek force-pushed the trezor-withdraw-modal branch from cdead7b to ef6f94f Compare January 7, 2025 10:55

tomasklim approved these changes Jan 7, 2025

View reviewed changes

tomasklim mentioned this pull request Jan 7, 2025

Trezor withdraw modal #16228

Merged

tomasklim closed this Jan 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: cardano modal with governance #16003

feat: cardano modal with governance #16003

vladimirvolek commented Dec 17, 2024 •

edited

Loading

tomasklim left a comment •

edited

Loading

enjojoy commented Jan 2, 2025 •

edited

Loading

enjojoy commented Jan 2, 2025 •

edited

Loading

vladimirvolek commented Jan 3, 2025

enjojoy commented Jan 3, 2025

mroz22 Jan 3, 2025

mroz22 Jan 3, 2025

vladimirvolek Jan 3, 2025

vladimirvolek commented Jan 3, 2025

mroz22 commented Jan 3, 2025 •

edited

Loading

vladimirvolek commented Jan 5, 2025

adamhavel commented Jan 5, 2025

vladimirvolek commented Jan 5, 2025

mroz22 commented Jan 6, 2025 •

edited

Loading

vladimirvolek commented Jan 7, 2025

tomasklim commented Jan 7, 2025

tomasklim commented Jan 7, 2025

feat: cardano modal with governance #16003

feat: cardano modal with governance #16003

Conversation

vladimirvolek commented Dec 17, 2024 • edited Loading

tomasklim left a comment • edited Loading

Choose a reason for hiding this comment

enjojoy commented Jan 2, 2025 • edited Loading

enjojoy commented Jan 2, 2025 • edited Loading

vladimirvolek commented Jan 3, 2025

enjojoy commented Jan 3, 2025

mroz22 Jan 3, 2025

Choose a reason for hiding this comment

mroz22 Jan 3, 2025

Choose a reason for hiding this comment

vladimirvolek Jan 3, 2025

Choose a reason for hiding this comment

vladimirvolek commented Jan 3, 2025

mroz22 commented Jan 3, 2025 • edited Loading

vladimirvolek commented Jan 5, 2025

adamhavel commented Jan 5, 2025

vladimirvolek commented Jan 5, 2025

mroz22 commented Jan 6, 2025 • edited Loading

vladimirvolek commented Jan 7, 2025

tomasklim commented Jan 7, 2025

tomasklim commented Jan 7, 2025

vladimirvolek commented Dec 17, 2024 •

edited

Loading

tomasklim left a comment •

edited

Loading

enjojoy commented Jan 2, 2025 •

edited

Loading

enjojoy commented Jan 2, 2025 •

edited

Loading

mroz22 commented Jan 3, 2025 •

edited

Loading

mroz22 commented Jan 6, 2025 •

edited

Loading