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: shorten "From" in details #336

Draft
wants to merge 8 commits into
base: development
Choose a base branch
from

Conversation

zugdev
Copy link
Contributor

@zugdev zugdev commented Oct 14, 2024

Resolves #312

Resolves #237

@zugdev zugdev requested a review from Keyrxng as a code owner October 14, 2024 21:17
@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Oct 14, 2024

Copy link
Contributor

github-actions bot commented Oct 14, 2024

@0x4007
Copy link
Member

0x4007 commented Oct 14, 2024

I think this should only shorten when needed. On full desktop view it is not needed but it is still shortened.

@zugdev
Copy link
Contributor Author

zugdev commented Oct 14, 2024

I think this should only shorten when needed. On full desktop view it is not needed but it is still shortened.

I know that, but for me the "For" in main collect display is also shortened therefore I didn't know. Will fix both.

@zugdev
Copy link
Contributor Author

zugdev commented Oct 15, 2024

This is a fancy solution

@zugdev
Copy link
Contributor Author

zugdev commented Oct 15, 2024

@0x4007 should be good 2 go

@zugdev
Copy link
Contributor Author

zugdev commented Oct 15, 2024

Here is a video demo:

shortenedAddress.mp4

@rndquu rndquu self-requested a review October 15, 2024 08:24
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

Works fine. Although the js approach seems to be kind of an overkill for a task of shortening strings, I suppose we could simply apply css to shorten addresses at the end.

@zugdev
Copy link
Contributor Author

zugdev commented Oct 15, 2024

I've added ens name handling too to fix #237. Demo:

ens-demo.webm

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

This should be simplified using CSS

return address;
}

// remove 1 letter for every 6px below 520px
Copy link
Member

Choose a reason for hiding this comment

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

Please use pure CSS. I implemented this logic in work.ubq.fi for the issue titles and repository names. You can probably use the same logic.

function formatLargeNumber(value: BigNumber, decimals: number): string {
const num = parseFloat(ethers.utils.formatUnits(value, decimals));

if (num >= 1_000_000_000_000_000) {
return "Unlimited"; // we can consider quintillion and above basically unlimited
return "Unlimited"; // we can consider quintillion and above basically unlimited
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return "Unlimited"; // we can consider quintillion and above basically unlimited
return "Unlimited"; // we can consider quadrillion and above basically unlimited

@0x4007
Copy link
Member

0x4007 commented Oct 16, 2024

Normally resolving multiple issues with a single pull is forbidden but to be honest both should have bene consolidated into a single task anyways so will let that slide!

Can you implement using CSS? It will require less logic and we don't need to clutter our typescript further.

@zugdev
Copy link
Contributor Author

zugdev commented Oct 16, 2024

Can you implement using CSS? It will require less logic and we don't need to clutter our typescript further.

Yeah will refac today

@rndquu rndquu marked this pull request as draft October 16, 2024 11:45
@zugdev
Copy link
Contributor Author

zugdev commented Oct 18, 2024

@0x4007 in work.ubq.fi you use text-overflow: ellipsis this is not good practice for addresses since the ellipsis is at the end of the word. Addresses would look like: "0xb1920a2...", but we probably want to show the end of the address as well as in "0xb192...b123". The same idea for ens, we always want to show the end particle: 'long...ens.eth". There is no easy css solution to do that. Do you prefer the simpler css solution, hiding the end of the address/ens or the typescript slicing one?

@0x4007
Copy link
Member

0x4007 commented Oct 18, 2024

text-directon: rtl with two elements solves that

@zugdev
Copy link
Contributor Author

zugdev commented Oct 18, 2024

The trimming part of it is working though I can't make td respect the width of the table. It overflows to fit content going out of the screen as attached in the following screenshot:

image

I've searched and tried out some StackOverflow solutions. Will try it later, if anyone knows easily how to do that lmk, it also seems someone tried to fix td overflow before as well indicated by:

https://github.com/ubiquity/pay.ubq.fi/blob/0ad1138ce8ab3b641cdee844df74d3e31dcf7f8b/static/styles/rewards/claim-table.css#L78C1-L89C2

It sucks to fix this which is a limiter of implementing a css solution.

@zugdev
Copy link
Contributor Author

zugdev commented Oct 29, 2024

Still waiting on comments here, tbh the table is a generally bad HTML component. td always overflows and therefore there isn't much we can do with CSS only. I am still trying some stuff tho.

image

Copy link
Contributor

ubiquity-os bot commented Nov 4, 2024

@zugdev, this task has been idle for a while. Please provide an update.

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.

from field in #additionalDetails overflow Responsive issues on long recipient's name
3 participants