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: custom receive address overlay on long addies #8284

Merged
merged 8 commits into from
Dec 8, 2024

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Dec 5, 2024

Description

Does what it says on the box, see screenshots. No transparency, no problems.

Actually does a bit more than it says on the box:

  • Ensures text area input is slightly larger in custom receive address, by virtue of padding-end now being slightly smaller. This allows for a bit more text to be displayed gracefully
  • Makes inputs shrink in <AddressInput /> (i.e shared between custom receive address input, and regular sends inputs) for inputs larger than 42 chars (i.e your standard ETH addy) which is where we starting having problem from in custom receive address input. This allows user to see their full inputted receive address.
  • This alone made things look much better for custom receive address, but spotted another issue with regular sends in develop:
image image image image image

We were missing

  • around half a char space for EVM addies
  • ~9. chars space for bitcoincash: prefix addies
  • ~4 chars for SOL addies.

While making input shrinks starting from > 42 chars made things look better for the two latter, the former still missed a tiny bit of space

Fixed this by:

  1. Making pe a teeny tiny bit smaller (which effectively ensures a bit more textarea regardless of input)
  2. Making the fontSize in the shared component a teeny tiny bit smaller as well for the <= 42 chars case

Issue (if applicable)

closes #8276

Risk

High Risk PRs Require 2 approvals

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

None

Testing

Custom Receive address

  • Valid EVM addies display fully

  • Valid bitcoincash: addies display fully

  • Valid SOL addies display fully

  • Invalid EVM addies (replace last char with another) do not have buttons overlaying last chars

  • Invalid bitcoincash: addies (replace first char after bitcoincash: with another) do not have buttons overlaying last chars

  • Invalid SOL addies (replace first char with another) do not have buttons overlaying last chars

Sends

  • EVM addies display fully
  • bitcoincash: addies display fully
  • SOL addies display fully

Engineering

  • ^

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)
  • ^

Screenshots (if applicable)

See this diff (left) vs. develop (right)

Custom Receive address

  • Valid EVM addies display fully
image
  • Valid bitcoincash: addies display fully
image
  • Valid SOL addies display fully
image
  • Invalid EVM addies (replace last char with another) do not have buttons overlaying last chars
image
  • Invalid bitcoincash: addies (replace first char after bitcoincash: with another) do not have buttons overlaying last chars
image
  • Invalid SOL addies (replace first char with another) do not have buttons overlaying last chars
image

Sends

  • EVM addies display fully
image
  • bitcoincash: addies display fully
image
  • SOL addies display fully
image

@gomesalexandre gomesalexandre requested a review from a team as a code owner December 5, 2024 04:15
Copy link
Collaborator

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

It works from an external addy without error:
image

But it doesnt work for an external addy with an error as the button is transparent:

image

I would suggest that the button container has a background so even if the button is transparent we don't see the addy, wdyt?

@gomesalexandre
Copy link
Contributor Author

gomesalexandre commented Dec 6, 2024

I would suggest that the button container has a background so even if the button is transparent we don't see the addy, wdyt?

Couldn't find a way to make this look nice, so went with an alternative direction here of

  1. making the text input and buttons not overlay altogether
  2. using programmatic fontSize for <AddressInput />'s <Input /> based on text length
Screenshot 2024-12-06 at 12 08 56

418231d
See https://jam.dev/c/7736bb6c-7edc-43d1-af59-5dc9a7e4dc5b

Additionally, also made the text area a teeny tiny larger 🚲 for regular sends, since I noticed it also crops thing a bit for EVM addies in develop, and I didn't want to change the length heuristics to be 41 and make the text smaller both in sends and custom receive address just for the sole purpose of adding this half-char of space missing.

377147e
See develop vs. this diff:
https://jam.dev/c/d0bb1674-9e4e-42ff-880a-7a11d2d4cdbd

Also pushed a final commit re: making the default fontSize for <=42 chars in the shared component being 0.5px smaller, ensuring no rugs in EVM sends:

See before:

image

// This is already a `useCallback()`
// eslint-disable-next-line react-memo/require-usememo
sx={{
fontSize: value && value.length > 42 ? '12px' : '13.5px',
Copy link
Contributor Author

@gomesalexandre gomesalexandre Dec 6, 2024

Choose a reason for hiding this comment

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

0.5px smaller than the regular 14px sm to accomodate for EVM sends looking slightly off if using 14px

See 14px (sm)

image

vs. 13.5px

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

The only thing that could be weird is when typing manually, but it should never happen, and if it happens it's weird anyway, so it looks acceptable to have a weird size changing when typing 👀

@@ -108,6 +108,7 @@ export const Address = () => {
{translate('modals.send.sendForm.sendTo')}
</FormLabel>
<AddressInput
pe={'9.5'}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

9.5 vs. the default 10 in <AddressInput /> to give this a lil less padding-end, meaning a lil bit more textarea

Copy link
Collaborator

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

EVM Desktop

https://jam.dev/c/889f37b5-ca9a-4530-8de9-be2a51ec1406

Solana Desktop

https://jam.dev/c/e0362e5a-571e-4c74-8d86-c9b83672f7d3

Unforunately, on mobile it doesn't work, at least for the send input:

SEND:
image

SWAP EXTERNAL:
image

@gomesalexandre
Copy link
Contributor Author

Unforunately, on mobile it doesn't work, at least for the send input:

Improved for mobile in 6cc2932:

image image image image image image

Note, for I didn't feel like it was worth going lower than 9.8px for the sole purpose of handling bitcoincash: prefixed addies for send mobile as users can understand the prefix clearly anyway.
Same rationale for the custom receive address mobile screenshots with cursor at the end of the selection hiding the start, at this point things just would become too small, so I believe this is the best tradeoff!

Desktop regression test from last diff:

image image image

Note @NeOMakinG as noted previously, react-device-detect doesn't react to viewport changes so make sure to refresh between mobile and desktop testing

Copy link
Collaborator

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

https://jam.dev/c/4ef7c5f7-69a1-4443-a76e-9887cf64704d

Desktop solana send:
image

Mobile:
image

Looks good now!

@gomesalexandre gomesalexandre enabled auto-merge (squash) December 7, 2024 00:44
@0xApotheosis 0xApotheosis disabled auto-merge December 8, 2024 21:50
@0xApotheosis 0xApotheosis merged commit e9f4855 into develop Dec 8, 2024
3 checks passed
@0xApotheosis 0xApotheosis deleted the fix_receive_overlay branch December 8, 2024 21:50
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.

Manual receive address allocation in trade modal too short for SOL address
3 participants