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

feat: create simple Arbitrary Txs action #3624

Open
wants to merge 10 commits into
base: feat/arbitrary-txs
Choose a base branch
from

Conversation

Nortsova
Copy link
Contributor

@Nortsova Nortsova commented Nov 4, 2024

Description

Figma designs: https://www.figma.com/design/5V8pr7iMwXsT9L3VAZsmUt/Actions-%26-Motions?node-id=24968-204505&node-type=frame&t=VK5oE9wW19JFFa0k-0
Feature spec: https://www.notion.so/colony/Arbitrary-Transactions-ba4b096b3b664cb09ef65ad35c4331fa

Add Arbitrary transaction action type to the action form alongside all the required fields and the Contract interaction modal that shows on form submission.

Note: This is UI only, it doesn't need to be wired to any sagas at this point.

Testing

Step 1. Select "Arbitrary transaction" Motion (usually I select simple payment and then choose Arbitrary Transaction motion)
image

Step 2. Check that this form has validation
image

Step 3. Fill in the basic fields
Step 4. Press "Add transaction"
Step 5. Verify that you can see Modal and this Modal matches the design
image

Note

Note that this is a UI-only task, and some of the fields will be dynamic.
"Unformat JSON" button will be added in the next iteration as well

Step 6. Fill this form with the correct address, 0xc9B6218AffE8Aba68a13899Cbf7cF7f14DDd304C, because this Modal doesn't have validation yet, and we want to test how the table will look in the next step

Step 7. After you submit this form, Modal should be closed

Step 8. Verify that the Table matches the design

image

Step 9. Submit the form and verify that you can see the form values in the console

image

Motion forms are pretty complicated, and I worked with them for the first time, so comments are very welcome here 🙌

Diffs

Changes 🏗

  • I removed FormSelect and FormInput from C2F to common components (forms, in general are a pain, and we should revisit them one day 🥲 )
  • Added mode and label prop to Textarea component
  • I moved some stuff from UserSelect to general components so I can reuse it 🙌
  • getMaskedAddress util was added

Contributes to #3532

@Nortsova Nortsova self-assigned this Nov 4, 2024
@Nortsova Nortsova requested review from a team as code owners November 4, 2024 19:11
@Nortsova Nortsova marked this pull request as draft November 4, 2024 19:11
Comment on lines 15 to 19
contract: string().defined(),
json: string().defined(),
method: string().defined(),
amount: string().defined(),
to: string().defined(),
Copy link
Contributor Author

@Nortsova Nortsova Nov 5, 2024

Choose a reason for hiding this comment

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

this is temporary, in the next iterations we will update these fields to be dynamic (according to ABI from the contract)


export type ArbitraryActionTypes = ActionTypeWithPayload<
ActionTypes.CREATE_ARBITRARY_TRANSACTION,
any
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one will be updated during sagas work

@Nortsova Nortsova force-pushed the feat/3532-motion-arbitrary-txs branch from c7bff3a to a31ec3b Compare November 5, 2024 21:25
Comment on lines +28 to +60
return isUserAddressValid ? (
<>
<UserAvatar
userName={userName}
userAddress={address}
userAvatarSrc={userAvatarSrc}
size={size}
{...rest}
/>
<p
className={clsx('ml-2 truncate text-md font-medium', {
'text-warning-400': !isVerified,
'text-gray-900': isVerified,
})}
>
{title ?? address}
</p>
{isVerified && (
<CircleWavyCheck
size={14}
className="ml-1 flex-shrink-0 text-blue-400"
/>
)}
</>
) : (
<div className="flex items-center gap-1 text-negative-400">
<WarningCircle size={16} />
<span className="text-md">
{formatText({
id: 'actionSidebar.addressError',
})}
</span>
</div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one just a part of the logic from UserSelect

Comment on lines 50 to 61
{
title: 'Method',
value: original.method,
},
{
title: '_to (address)',
value: original.to,
},
{
title: '_amount (uint256)',
value: original.amount,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will be dynamic soon 🔜

@jakubcolony jakubcolony changed the title [WIP]: feat: create simple Arbitrary Txs motion [WIP]: feat: create simple Arbitrary Txs action Nov 5, 2024
@Nortsova Nortsova marked this pull request as ready for review November 5, 2024 21:45
Copy link
Collaborator

@jakubcolony jakubcolony left a comment

Choose a reason for hiding this comment

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

Very nice work @Nortsova, thanks for picking this up.

I only spotted a few minor issues:

There is too much space above the button when the table is empty:
image


In Figma, the textarea stops growing at 166px height when it becomes scrollable:
image
image


Mobile view:

  • Long values overflow and go off the screen
  • The address should be formatted as 0xXXXX...XXXX as per Figma designs
image

Validation ✅

image

Transactions table ✅
image


Submitting form ✅
image

Mobile view ✅
image
image

@@ -133,6 +133,18 @@ const useActionsList = () => {
// },
],
},
{
key: '6',
Copy link
Collaborator

Choose a reason for hiding this comment

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

@arrenv Is there a preference regarding which group arbitrary transaction should show under?

image

src/graphql/generated.ts Outdated Show resolved Hide resolved
src/redux/sagas/actions/arbitrationTxs.ts Outdated Show resolved Hide resolved
src/redux/types/actions/arbitrary.ts Outdated Show resolved Hide resolved
@Nortsova Nortsova changed the title [WIP]: feat: create simple Arbitrary Txs action feat: create simple Arbitrary Txs action Nov 6, 2024
@Nortsova
Copy link
Contributor Author

Nortsova commented Nov 6, 2024

Thank you, @jakubcolony, for your review. I really appreciate your help :)

I made all the changes you requested 🙌 So please check it again in your free time.

@jakubcolony jakubcolony self-requested a review November 6, 2024 15:54
Copy link
Collaborator

@jakubcolony jakubcolony left a comment

Choose a reason for hiding this comment

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

Thanks for such a quick turnaround! 💯

Looks like we're nearly there, just noticing the textarea has collapsed to a single line and it doesn't grow as I enter extra lines:

image
Screen.Recording.2024-11-06.at.17.39.03.mov

In Figma, both empty and filled textarea have a height of 166px, so we should set it to that in all cases:

image

All previously reported snags are resolved:

Button spacing ✅
image

Address masking ✅
image

Mobile view ✅
image

@Nortsova
Copy link
Contributor Author

Nortsova commented Nov 7, 2024

Thank you @jakubcolony for testing 🙌

There are probably some issues with height/textarea. I put a class with a height of 10.25rem, but your browser doesn't pick it up for some reason. Nice catch! I covered that case with rows={7}

So now, if the browser does not pick the height from styles, it will calculate the height based on rows={7}; otherwise, it will take 10.25rem.

The only difference here is that rows={7} height could be slightly smaller than with height 10.25rem.

rows:
image

height style:
image

Please recheck it, and if it is still an issue - let me know 🙌

@jakubcolony jakubcolony self-requested a review November 7, 2024 11:02
Copy link
Collaborator

@jakubcolony jakubcolony left a comment

Choose a reason for hiding this comment

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

That's interesting, what browser are you using?

It looks good on my end now, great work @Nortsova! 💪

image image

@Nortsova
Copy link
Contributor Author

Nortsova commented Nov 7, 2024

I am happy that it works now 🎉 Actually, after your comment, I tested Chrome, Safari, and Firefox, and for all of them, it works without adding a row 😕 I don't know exactly why it happened on your machine 😄

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.

2 participants