fix: add overview info and change copy for advanced payment review #3724

adam-strzelec · 2024-11-15T16:43:23Z

Description

Change copy and add overview info for advancep payment

Testing

Create advanced payment
Go to Review tab

Diffs

Permissions:

Staking:

Creator:

Main issue - #3680

CLAassistant · 2024-11-15T16:43:29Z

All committers have signed the CLA.

iamsamgibbs

Thanks for your work on this. I can see the updated copy for the creator is correct.

However, I created the action using permissions rather than staking, so I don't think a stake amount should be shown here:

The original issue says "The card should show a different message about how they created the payment, i.e. Permissions, or Stake amount." Although the figma design doesn't seem to include the permissions version.

The PaymentBuilderWidget is really getting bloated now, do you think you could move all the review step logic into a new component to keep things a bit tidier?

adam-strzelec · 2024-11-20T08:58:39Z

@iamsamgibbs I can't create an action for Stake. I also don't know how designs should look for Permissions

iamsamgibbs · 2024-11-20T09:31:15Z

@iamsamgibbs I can't create an action for Stake.

You might need to rebase, this PR fixes stakes and got merged yesterday: #3629

I also don't know how designs should look for Permissions

@arrenv @melyndav Might need a quick figma mock up / description for how this should look (assuming we've understood correctly what is required here)

jakubcolony · 2024-12-03T00:11:29Z

@adam-strzelec The only difference between Permissions and Staking decision methods is that the stake amount should only be shown for the latter. If a payment was created with permissions, non-creators should only see the Payment creator field on this card:

iamsamgibbs

Nice job, this is looking a lot better now! Whilst in review and viewing as another user:

As the creator:

Permissions:

Staked:

However, the wording in the completed view of these steps does not match the figma design, it should read "Payment creator completed the review step."

Currently looks like this:

I think with that small change we're good to go!

arrenv · 2024-12-09T15:42:46Z

@arrenv @melyndav Might need a quick figma mock up / description for how this should look (assuming we've understood correctly what is required here)

@adam-strzelec there will just not be any stake amount row. Figma design.

adam-strzelec · 2024-12-12T11:23:30Z

@arrenv @melyndav Might need a quick figma mock up / description for how this should look (assuming we've understood correctly what is required here)

@adam-strzelec there will just not be any stake amount row. Figma design.

@arrenv @iamsamgibbs That's how it works now

jakubcolony

Thanks for your PR @adam-strzelec. The copy looks good, however I noted the bottom padding of the card is too big compared to Figma:

I also left a comment where a different field should be used to access payment creator address.

As creator:

As a different user:

jakubcolony · 2024-12-16T21:54:17Z

...mpletedAction/partials/PaymentBuilder/partials/PaymentBuilderWidget/PaymentBuilderWidget.tsx

@@ -82,7 +85,7 @@ const PaymentBuilderWidget: FC<PaymentBuilderWidgetProps> = ({ action }) => {
    setSelectedReleaseAction,
  } = usePaymentBuilderContext();

-  const { expenditureId } = action;
+  const { expenditureId, initiatorUser } = action;


It's possible initiatorUser will be null, for example as a result of direct chain interaction outside the UI.

Wherever expenditure creator's address is referenced it should use action.initiatorAddress.

… for both

mmioana

Nice work on this one @adam-strzelec 🙌

View after the Advanced payment was created as the Payment creator

View after the Advanced payment was created as another user

View after the payment creator finalised the the Review step.

However as @iamsamgibbs pointed out, the widget header copy after the Review step has been completed doesn't match Figma, so would appreciate it if we could update this as well @adam-strzelec 🥇

jakubcolony

The copy and padding looks good now, thank you @adam-strzelec. However, I noticed an unsafe array access, which should hopefully be easy to fix.

jakubcolony · 2025-01-14T21:58:52Z

...mpletedAction/partials/PaymentBuilder/partials/PaymentBuilderWidget/PaymentBuilderWidget.tsx

@@ -92,6 +95,11 @@ const PaymentBuilderWidget: FC<PaymentBuilderWidgetProps> = ({ action }) => {
    stopPolling,
  } = useGetExpenditureData(expenditureId, { pollUntilUnmount: true });

+  const tokenData = getSelectedToken(
+    colony,
+    expenditure?.slots?.[0].payouts?.[0].tokenAddress ?? '',


I hadn't caught it earlier, the array indexes need to be followed by ? too, otherwise this will crash on empty slots/payouts array.

Suggested change

expenditure?.slots?.[0].payouts?.[0].tokenAddress ?? '',

expenditure?.slots?.[0]?.payouts?.[0]?.tokenAddress ?? '',

davecreaser

Looking good from my side @adam-strzelec, once the empty array fix Jakub raised is dealt with!

Permissions:

Staking:

Ready to approve as soon as that potential crash is fixed 🚀

mmioana

Hey @adam-strzelec, I noticed the header issue is still present and not matching Figma when the Review step is completed. Is this something we don't want to tackle for now?

Also @adam-strzelec can you please address @jakubcolony comment regarding the empty arrays?

adam-strzelec · 2025-01-23T17:49:49Z

@mmioana Did you mean to change the Status Badge to one that corresponds to the currently selected step?

mmioana · 2025-01-27T14:25:14Z

Hey @adam-strzelec! Thanks for your patience 🙏 I meant the card header after the Review step has been completed currently says Member used permissions/staking to create this action.
While in Figma

adam-strzelec · 2025-01-28T13:09:04Z

@mmioana I see that it was already fixed in this commit 6b7e3cd
On my device it shows header properly 🙂

mmioana

@adam-strzelec must be I haven't pulled the latest changes, but can confirm that now it properly shows up also for me! Thank you for all the effort on this issue 🥇

Screen.Recording.2025-01-28.at.19.58.25.mov

Screen.Recording.2025-01-28.at.19.59.03.mov

iamsamgibbs

Nice job handling all of these cases! This is good to go now.

Permissions as the creator:

Permissions as another user:

Permissions after going to the next step:

Staking as the creator:

Staking as another user:

Staking after moving to the next step:

jakubcolony

All good apart from a single question mark - please ping me once you add it and I'll approve straightaway.

jakubcolony · 2025-02-04T21:49:16Z

...mpletedAction/partials/PaymentBuilder/partials/PaymentBuilderWidget/PaymentBuilderWidget.tsx

@@ -92,6 +95,11 @@ const PaymentBuilderWidget: FC<PaymentBuilderWidgetProps> = ({ action }) => {
    stopPolling,
  } = useGetExpenditureData(expenditureId, { pollUntilUnmount: true });

+  const tokenData = getSelectedToken(
+    colony,
+    expenditure?.slots?.[0].payouts?.[0]?.tokenAddress ?? '',


You're missing a ? just before .payouts, which means the app can still crash if expenditure slots are not set yet:

iamsamgibbs

Thanks for the tiny fix, all good here.

mmioana

Thank you a lot for your continuous effort on this issue @adam-strzelec 🙌

Looks good from my side 💯

Using permissions

Screen.Recording.2025-02-07.at.14.10.05.mov

Using staking

Screen.Recording.2025-02-07.at.14.11.06.mov

jakubcolony

Thanks for the fix, all good here 👍

adam-strzelec self-assigned this Nov 15, 2024

adam-strzelec requested a review from a team as a code owner November 15, 2024 16:43

iamsamgibbs requested changes Nov 18, 2024

View reviewed changes

adam-strzelec force-pushed the fix/16167998-advancep-payment-review branch from 6c758b5 to 7537056 Compare November 21, 2024 09:03

AdrianDudko linked an issue Nov 22, 2024 that may be closed by this pull request

Advanced payment review without permissions #3680

Closed

adam-strzelec force-pushed the fix/16167998-advancep-payment-review branch from 7537056 to b17624f Compare December 3, 2024 10:19

adam-strzelec requested a review from iamsamgibbs December 3, 2024 10:26

iamsamgibbs requested changes Dec 6, 2024

View reviewed changes

adam-strzelec requested a review from iamsamgibbs December 12, 2024 17:41

jakubcolony requested changes Dec 16, 2024

View reviewed changes

strzelec and others added 4 commits December 19, 2024 11:45

fix: add overview info and change copy for advanced payment review

718c213

fix: display overview for staking

6a290db

fix: display stake amount only for staked actions and payment creator…

7d9d9dc

… for both

fix: code remove margin and initiator user

fa4d0e0

adam-strzelec force-pushed the fix/16167998-advancep-payment-review branch from cc22cbd to fa4d0e0 Compare December 19, 2024 11:18

adam-strzelec requested a review from jakubcolony December 19, 2024 11:19

mmioana requested changes Dec 30, 2024

View reviewed changes

fix: review title copy

6b7e3cd

adam-strzelec requested a review from mmioana December 31, 2024 12:11

jakubcolony requested changes Jan 14, 2025

View reviewed changes

davecreaser suggested changes Jan 17, 2025

View reviewed changes

mmioana requested changes Jan 20, 2025

View reviewed changes

adam-strzelec requested a review from jakubcolony January 23, 2025 17:50

adam-strzelec requested review from mmioana and davecreaser January 23, 2025 17:50

adam-strzelec marked this pull request as draft January 28, 2025 08:53

fix: handle undefined array

57ad081

adam-strzelec force-pushed the fix/16167998-advancep-payment-review branch from 63dd476 to 57ad081 Compare January 28, 2025 13:06

adam-strzelec marked this pull request as ready for review January 28, 2025 13:09

mmioana previously approved these changes Jan 28, 2025

View reviewed changes

iamsamgibbs previously approved these changes Jan 29, 2025

View reviewed changes

jakubcolony requested changes Feb 4, 2025

View reviewed changes

fix: handle array

8945a77

adam-strzelec dismissed stale reviews from iamsamgibbs and mmioana via 8945a77 February 7, 2025 10:36

adam-strzelec requested review from jakubcolony, mmioana and iamsamgibbs February 7, 2025 10:37

iamsamgibbs approved these changes Feb 7, 2025

View reviewed changes

mmioana approved these changes Feb 7, 2025

View reviewed changes

jakubcolony approved these changes Feb 7, 2025

View reviewed changes

adam-strzelec merged commit 8d919d1 into master Feb 7, 2025
2 checks passed

adam-strzelec deleted the fix/16167998-advancep-payment-review branch February 7, 2025 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add overview info and change copy for advanced payment review #3724

fix: add overview info and change copy for advanced payment review #3724

adam-strzelec commented Nov 15, 2024 •

edited

Loading

CLAassistant commented Nov 15, 2024 •

edited

Loading

iamsamgibbs left a comment

adam-strzelec commented Nov 20, 2024

iamsamgibbs commented Nov 20, 2024

jakubcolony commented Dec 3, 2024

iamsamgibbs left a comment

arrenv commented Dec 9, 2024 •

edited

Loading

adam-strzelec commented Dec 12, 2024 •

edited

Loading

jakubcolony left a comment

jakubcolony Dec 16, 2024

mmioana left a comment

jakubcolony left a comment

jakubcolony Jan 14, 2025

davecreaser left a comment

mmioana left a comment

adam-strzelec commented Jan 23, 2025

mmioana commented Jan 27, 2025

adam-strzelec commented Jan 28, 2025

mmioana left a comment

iamsamgibbs left a comment

jakubcolony left a comment

jakubcolony Feb 4, 2025

iamsamgibbs left a comment

mmioana left a comment

jakubcolony left a comment

	expenditure?.slots?.[0].payouts?.[0].tokenAddress ?? '',
	expenditure?.slots?.[0]?.payouts?.[0]?.tokenAddress ?? '',

fix: add overview info and change copy for advanced payment review #3724

fix: add overview info and change copy for advanced payment review #3724

Conversation

adam-strzelec commented Nov 15, 2024 • edited Loading

Description

Testing

Diffs

CLAassistant commented Nov 15, 2024 • edited Loading

iamsamgibbs left a comment

Choose a reason for hiding this comment

adam-strzelec commented Nov 20, 2024

iamsamgibbs commented Nov 20, 2024

jakubcolony commented Dec 3, 2024

iamsamgibbs left a comment

Choose a reason for hiding this comment

arrenv commented Dec 9, 2024 • edited Loading

adam-strzelec commented Dec 12, 2024 • edited Loading

jakubcolony left a comment

Choose a reason for hiding this comment

jakubcolony Dec 16, 2024

Choose a reason for hiding this comment

mmioana left a comment

Choose a reason for hiding this comment

jakubcolony left a comment

Choose a reason for hiding this comment

jakubcolony Jan 14, 2025

Choose a reason for hiding this comment

davecreaser left a comment

Choose a reason for hiding this comment

mmioana left a comment

Choose a reason for hiding this comment

adam-strzelec commented Jan 23, 2025

mmioana commented Jan 27, 2025

adam-strzelec commented Jan 28, 2025

mmioana left a comment

Choose a reason for hiding this comment

iamsamgibbs left a comment

Choose a reason for hiding this comment

jakubcolony left a comment

Choose a reason for hiding this comment

jakubcolony Feb 4, 2025

Choose a reason for hiding this comment

iamsamgibbs left a comment

Choose a reason for hiding this comment

mmioana left a comment

Choose a reason for hiding this comment

jakubcolony left a comment

Choose a reason for hiding this comment

adam-strzelec commented Nov 15, 2024 •

edited

Loading

CLAassistant commented Nov 15, 2024 •

edited

Loading

arrenv commented Dec 9, 2024 •

edited

Loading

adam-strzelec commented Dec 12, 2024 •

edited

Loading