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

Fix: Truncate Search Item label text #4160

Merged
merged 2 commits into from
Jan 29, 2025
Merged

Conversation

rumzledz
Copy link
Contributor

@rumzledz rumzledz commented Jan 22, 2025

Description

Just a good ol' combination of text and address truncation.

truncate-address

Testing

Important

1. Update amy's username

  1. Open the UserAccountPage/hooks.tsx file and update line 31 to:
return { canChangeUsername: true, daysTillUsernameChange };
  1. Connect amy's wallet
  2. Visit your profile page: http://localhost:9091/account/profile
  3. Update your username to:
QQQQQQQQQQQQQQQQQQQQQQQQQQQQQQ

2. Add a wallet address to the list of recippients

  1. Visit the Wayne colony: http://localhost:9091/wayne
  2. Copy its address
  3. Visit the Planex colony: http://localhost:9091/planex
  4. Bring up the Manage verified members form
  5. Add Wayne's address and submit the form

Verifying the truncation

  1. Bring up the Simple payment form
  2. Bring up the recipients list popover
  3. Verify that:
  • the popover doesn't have a horizontal scrollbar
  • your new username is truncated
  • Wayne's contract address conforms to our address truncation
  1. Change the action type to Manage Verified Members form
  2. Set the Add/remove field to Remove members
  3. Click the Select members field
  4. Verify that:
  • the popover doesn't have a horizontal scrollbar
  • your new username is truncated
  • Wayne's contract address conforms to our address truncation

Resolves #3679

@rumzledz rumzledz self-assigned this Jan 22, 2025
@rumzledz rumzledz force-pushed the fix/3679-truncate-address branch from 2b2e1eb to d408621 Compare January 22, 2025 12:10
@rumzledz rumzledz marked this pull request as ready for review January 22, 2025 12:14
@rumzledz rumzledz requested a review from a team as a code owner January 22, 2025 12:14
Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

Nice work on this so far, it is very nearly there!

Simple payments recipient field looks good in both cases:

Screenshot 2025-01-24 at 12 19 23

Screenshot 2025-01-24 at 12 19 27

Wallet address for verified members works:

Screenshot 2025-01-24 at 12 20 51

But long usernames are not being truncated for verified members:

Screenshot 2025-01-24 at 12 20 10

I also noticed the bug resolved by #3733 is back, I'm hoping it's just that this PR hasn't been rebased since that was merged! 😬 I don't see anything here that would have messed with that.

Screenshot 2025-01-24 at 12 22 58

@rumzledz rumzledz requested a review from iamsamgibbs January 24, 2025 15:22
@rumzledz rumzledz force-pushed the fix/3679-truncate-address branch from d408621 to 23b6913 Compare January 24, 2025 15:22
@rumzledz rumzledz requested a review from a team January 24, 2025 15:22
Copy link
Contributor

@davecreaser davecreaser left a comment

Choose a reason for hiding this comment

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

Nice one @rumzledz!

All looks good with the original bug:
Screenshot 2025-01-24 at 17 15 00
Screenshot 2025-01-24 at 17 15 21

Just having some trouble / questions about the change to not return addresses when searching verified members.

Screenshot 2025-01-24 at 17 17 40

Seems to me that in the PR from @iamsamgibbs it states that addresses should show in the search for verified members?

@@ -160,7 +160,6 @@ const VerifiedMembersSelect: FC<VerifiedMembersSelectProps> = ({
'left-6 right-[1.9rem] w-auto': isMobile,
})}
showEmptyContent
shouldReturnAddresses
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should return addresses, right?

I added 0x94CB46d05c2c88A7A053632865634a19Ba4C02fc as a verified member, and when I search for it, it doesn't show up. If i remove the search term it does though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might need to revisit this one @iamsamgibbs 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

🫠 Strange! If it's in the list of options, it should return before even checking if shouldReturnAddresses is true or not (I also realise now that this name maybe isn't the most intuitive, when shouldReturnAddresses is true: if there are no matches in the list and the search value is address, then return the search value is the expected behaviour). Will join you in taking a look at this on Monday!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I just rebased from master and that seems to have fixed it. So something funny must've happened on master but got recently fixed or something 😅 @davecreaser @iamsamgibbs can I please ask you to try it again?

Copy link
Contributor Author

@rumzledz rumzledz Jan 28, 2025

Choose a reason for hiding this comment

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

Oh and @davecreaser we removed the shouldReturnAddresses prop for the VerifiedMembersSelect function because we don't want to show the unknown address item if it's not being returned from the search results. But only for the VerifiedMembersSelect component!

@rumzledz rumzledz force-pushed the fix/3679-truncate-address branch from 23b6913 to 25b4650 Compare January 28, 2025 11:24
@rumzledz rumzledz requested review from davecreaser and a team January 28, 2025 11:25
Copy link
Contributor

@davecreaser davecreaser left a comment

Choose a reason for hiding this comment

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

Nice one @rumzledz

Everything is still working, and the address now shows up in the search!

Screenshot 2025-01-28 at 12 05 36 Screenshot 2025-01-28 at 12 05 46 Screenshot 2025-01-28 at 12 07 23 Screenshot 2025-01-28 at 12 08 35

Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

Nice job, this is all working great now!

Adding an address:
Screenshot 2025-01-28 at 18 15 23

Trying to remove an address when it is not already a verified member:
Screenshot 2025-01-28 at 18 15 16

Removing an address that HAS actually been verified:
Screenshot 2025-01-28 at 18 22 07

And simple payments:
Screenshot 2025-01-28 at 18 22 21

Screenshot 2025-01-28 at 18 22 25

Lets get it merged!

Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

Approving so you can merge. Nicely done!

@rumzledz rumzledz merged commit e769d17 into master Jan 29, 2025
2 checks passed
@rumzledz rumzledz deleted the fix/3679-truncate-address branch January 29, 2025 08:59
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.

[Payments - QA] Display colony address according to design in payments
4 participants