search

ubiquity / pay.ubq.fi

Generate and claim spender permits (EIP-2612)
https://pay.ubq.fi
8 stars 28 forks source link

fix: responsiveness on long reciept name #238

Open jordan-ae opened 3 weeks ago

jordan-ae commented 3 weeks ago

Resolves #237

Screenshot 2024-06-03 211940

rndquu commented 3 weeks ago

@jordan-ae Pls fix the build, otherwise works fine

ubiquibot-continuous-deploys[bot] commented 3 weeks ago
db026c9
github-actions[bot] commented 3 weeks ago
Preview Deployment
db026c97b70f7680980025685ed9437c89a26035
jordan-ae commented 3 weeks ago

@Keyrxng

I did not submit my review correctly, lesson learned.

You can see the overlapping of the buttons on mobile if you open the additionalDetails field.

@Keyrxng please can I get a screenshot because removing that line fixes both the overlapping on the additionalDetails side too. Screen Shot 2024-06-09 at 12 52 29

Keyrxng commented 3 weeks ago

In your screenshot you are using the NFT permit which contains more info than an ERC20 permit.

In your screenshot, how do you close the additional details? How do you claim? You should see these buttons underneath the dialogue uncovered and fully visible

Try it with the ERC20 permit as that is the permit that's actively been generated, the NFT is not quite yet.

As you open and close the additional details dialogue you will see that the dialogue overlaps the button you pressed to open it.

Keyrxng commented 3 weeks ago

As requested earlier and on TG @jordan-ae.

It may affect it in other ways but this stood out to me

with css:

with-css

without css:

removed-css

jordan-ae commented 3 weeks ago

Really not sure what's up but this shows up okay for me. I tested the impact of removing that line of code and the only apparent one was that the code won't overflow anymore I've tested the display on multiple screen sizes and have not had anything breaking.

Screenshot 2024-06-09 140529

jordan-ae commented 3 weeks ago

Screen Shot 2024-06-09 at 12 52 29

@0x4007 I also dont know if its intentional or its a bug, but when viewing an NFT permit there's no way to navigate back from the details page.

Keyrxng commented 3 weeks ago

Really not sure what's up but this shows up okay for me. I tested the impact of removing that line of code and the only apparent one was that the code won't overflow anymore I've tested the display on multiple screen sizes and have not had anything breaking.

Screenshot 2024-06-09 140529

this specific overlap I'm talking about only happens on mobile

Keyrxng commented 3 weeks ago

Screen Shot 2024-06-09 at 12 52 29

0x4007 I also dont know if its intentional or its a bug, but when viewing an NFT permit there's no way to navigate back from the details page.

It is not intentional, it should behave in the exact same way that the ERC20 permit behaves. I will open a separate issue for it.

NFTs are not rolled out atm afaik so it's not high priority or app breaking

jordan-ae commented 3 weeks ago

Really not sure what's up but this shows up okay for me. I tested the impact of removing that line of code and the only apparent one was that the code won't overflow anymore I've tested the display on multiple screen sizes and have not had anything breaking. Screenshot 2024-06-09 140529

this specific overlap I'm talking about only happens on mobile

This is on mobile I can send in a full screenshot if that helps

Screenshot 2024-06-09 141555

Keyrxng commented 3 weeks ago

As requested earlier and on TG @jordan-ae.

It may affect it in other ways but this stood out to me

with css:

with-css

without css:

removed-css

You have to click additionalDetails and open the dialogue to repro this overlap, it does not overlap without interaction. Devtools, mobile-view and all templates show the overlap.

The spec targets the ENS name overflowing, the removal of the CSS is producing the overlap because the table is resizing itself rather than being fixed.

The difference in these two images is that the first ENS name is cortex.hiphop while the second one is cortex.hiphop x8

over

over-over

Note that an actual ENS name will be one string likely without spaces, I have added spaces between each here so that it wraps as expected.

8names

As opposed to the string you'll need to handle which is

image

jordan-ae commented 2 weeks ago

@Keyrxng thanks for the remarks. I noticed @0x4007 made the address to have an ellipsis when it gets to long. It didn't work because he forgot to add a max-width, so I've got that working now. So the sender address doesnt break and cause the glitch anymore. Before committing just wanted to get you guys opinion on if I should do an ellipsis on the front too or just make it wrap when the text gets too long.

0x4007 commented 2 weeks ago

Ellipsis generally is preferred but I am unsure if there are new problems caused by setting max-width on the table.

rndquu commented 2 weeks ago

@jordan-ae Pls mark this PR as "ready for review" when you're finished, as far as I understand the only question was regarding ellipsis which is answered here