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: responsiveness on long reciept name #238

Conversation

jordan-ae
Copy link
Contributor

Resolves #237

Screenshot 2024-06-03 211940

lib/chainlist Outdated Show resolved Hide resolved
@rndquu
Copy link
Member

rndquu commented Jun 4, 2024

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

@jordan-ae jordan-ae requested a review from 0x4007 June 5, 2024 12:38
@ubiquity-os-deployer
Copy link

Copy link
Contributor

github-actions bot commented Jun 6, 2024

Preview Deployment
db026c97b70f7680980025685ed9437c89a26035

Comment on lines -316 to -318
table * {
text-wrap: nowrap;
}
Copy link
Member

Choose a reason for hiding this comment

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

That is there for a reason. Have you seen how it breaks the page on mobile?

I estimated the task to be two hours, because it requires restructuring the table html.

Copy link
Member

Choose a reason for hiding this comment

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

My mistake it does overlap the buttons on mobile quite a bit more without this

Copy link
Member

@Keyrxng Keyrxng left a comment

Choose a reason for hiding this comment

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

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.

lib/chainlist Outdated Show resolved Hide resolved
Comment on lines -316 to -318
table * {
text-wrap: nowrap;
}
Copy link
Member

Choose a reason for hiding this comment

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

My mistake it does overlap the buttons on mobile quite a bit more without this

@jordan-ae
Copy link
Contributor Author

@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
Copy link
Member

Keyrxng commented Jun 9, 2024

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
Copy link
Member

Keyrxng commented Jun 9, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

Keyrxng commented Jun 9, 2024

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
Copy link
Member

Keyrxng commented Jun 9, 2024

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
Copy link
Contributor Author

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
Copy link
Member

Keyrxng commented Jun 9, 2024

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
Copy link
Contributor Author

@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
Copy link
Member

0x4007 commented Jun 11, 2024

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

@rndquu rndquu self-requested a review June 14, 2024 15:19
@rndquu rndquu marked this pull request as draft June 14, 2024 15:23
@rndquu
Copy link
Member

rndquu commented Jun 14, 2024

@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

@ubiquibot ubiquibot bot closed this Jul 1, 2024
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.

Responsive issues on long recipient's name
4 participants