-
Notifications
You must be signed in to change notification settings - Fork 850
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
address component showBoth prop #924
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great updates and thank you for test code!
I added some comments
And one more nit: when loading with showBoth
option component jumps a bit. It will cause to jump everything beyond the component. See also: https://web.dev/articles/cls
Screen.Recording.2024-09-05.at.11.18.28.mov
Thanks Rinat and Damu for review! pushed some commits.
Yeah I wan't sure how best we could fix the layout shift, but I added currently the skeleton for now : Screen.Recording.2024-09-07.at.7.16.54.PM.movSo if the address has ENS we show skeleton until fetching and then ens name instead of it. But if the ENS name is not present we fallback to normal address component(without showBoth) |
Good changes, thanks! Updated a bit:
Screen.Recording.2024-09-07.at.18.26.55.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm!
I think it's a good solution! I thought about something like fixing height of component, but it will add empty space around, when |
Thanks @rin-st makes sense! |
This looks great @technophile-04 !! <3 The only thing that does not quite convince me is the
IDK, just throwing it out there..... Happy to merge as it's too. |
Maybe something more explicit like showAddressAndEns? |
hmm, in #911 Austin said
So looks like we don't need all that props and But we have |
Just pushed a changes removing When you pass Here is the behaviour for default thing
Case 1 demo address as has ENS :Screen.Recording.2024-09-13.at.2.49.34.PM.movCase 2 Demo :Now here there will be a layout shift. The layout won't be that much visible if you don't pass any size eg: Screen.Recording.2024-09-13.at.2.49.56.PM.movhere is example test code :"use client";
import type { NextPage } from "next";
import { Address } from "~~/components/scaffold-eth";
const Home: NextPage = () => {
// address without ENS
// const demoAddress = "0x4e123738A66427ee9BD0C9dc67C09af397A7D93e";
// atg.eth address
const demoAddress = "0x34aA3F359A9D614239015126635CE7732c18fDF3";
return (
<>
<div className="flex items-center flex-col flex-grow pt-5">
<div className="px-5">
<div className="mb-12">
<h2 className="text-center text-3xl font-bold mb-4">Address Component Showcase</h2>
<div className="w-full flex items-center justify-center mb-10">
<Address address={demoAddress} />
</div>
<div className="grid grid-cols-1 md:grid-cols-2 gap-6">
<div>
<h3 className="text-xl font-semibold mb-2">Only Address or ENS</h3>
<div className="mb-4">
<p className="text-sm mb-1">Size: xs</p>
<Address size="xs" address={demoAddress} onlyEnsOrAddress />
</div>
<div className="mb-4">
<p className="text-sm mb-1">Size: sm</p>
<Address size="sm" address={demoAddress} onlyEnsOrAddress />
</div>
<div className="mb-4">
<p className="text-sm mb-1">Size: base(default)</p>
<Address size="base" address={demoAddress} onlyEnsOrAddress />
</div>
<div className="mb-4">
<p className="text-sm mb-1">Size: lg</p>
<Address size="lg" address={demoAddress} onlyEnsOrAddress />
</div>
<div className="mb-4">
<p className="text-sm mb-1">Size: xl</p>
<Address size="xl" address={demoAddress} onlyEnsOrAddress />
</div>
<div className="mb-4">
<p className="text-sm mb-1">Size: 2xl</p>
<Address size="2xl" address={demoAddress} onlyEnsOrAddress />
</div>
<div className="mb-4">
<p className="text-sm mb-1">Size: 3xl</p>
<Address size="3xl" address={demoAddress} onlyEnsOrAddress />
</div>
</div>
<div>
<h3 className="text-xl font-semibold mb-2">Default Address Behaviour</h3>
<div className="mb-4">
<p className="text-sm mb-1">Size: xs(default)</p>
<Address size="xs" address={demoAddress} />
</div>
<div className="mb-4">
<p className="text-sm mb-1">Size: sm</p>
<Address size="sm" address={demoAddress} />
</div>
<div className="mb-4">
<p className="text-sm mb-1">Size: base</p>
<Address size="base" address={demoAddress} />
</div>
<div className="mb-4">
<p className="text-sm mb-1">Size: lg</p>
<Address size="lg" address={demoAddress} />
</div>
<div className="mb-4">
<p className="text-sm mb-1">Size: xl</p>
<Address size="xl" address={demoAddress} />
</div>
<div className="mb-4">
<p className="text-sm mb-1">Size: 2xl</p>
<Address size="2xl" address={demoAddress} />
</div>
<div className="mb-4">
<p className="text-sm mb-1">Size: 3xl</p>
<Address size="3xl" address={demoAddress} />
</div>
</div>
</div>
</div>
</div>
</div>
</>
);
};
export default Home;
// dynamic: --------------------------
/* const Home: NextPage = () => {
const demoAddress = "0x34aA3F359A9D614239015126635CE7732c18fDF3";
const sizes: ("xs" | "sm" | "base" | "lg" | "xl" | "2xl" | "3xl")[] = ["xs", "sm", "base", "lg", "xl", "2xl", "3xl"];
return (
<>
<div className="flex items-center flex-col flex-grow pt-5">
<div className="px-5">
<div className="mb-12">
<h2 className="text-center text-3xl font-bold mb-4">Address Component Showcase</h2>
<div className="grid grid-cols-1 md:grid-cols-2 gap-6">
<div>
<h3 className="text-xl font-semibold mb-2">Without showBoth</h3>
{sizes.map(size => (
<div key={size} className="mb-4">
<p className="text-sm mb-1">Size: {size}</p>
<Address size={size} address={demoAddress} />
</div>
))}
</div>
<div>
<h3 className="text-xl font-semibold mb-2">With showBoth</h3>
{sizes.map(size => (
<div key={size} className="mb-4">
<p className="text-sm mb-1">Size: {size}</p>
<Address size={size} address={demoAddress} showBoth />
</div>
))}
</div>
</div>
</div>
</div>
</div>
</>
);
};
export default Home; */ |
Great, thanks! One more nit: after 8df4dbc Screen.Recording.2024-09-13.at.14.11.46.mov |
I played with it (a lot) and as I understand we have two options.
Screen.Recording.2024-09-17.at.21.48.19.mov
Screen.Recording.2024-09-17.at.23.21.33.movBoth cases uses default Pushed the second option since I think it looks better but feel free to update/revert if needed Page code (3 colons)
|
Thanks @rin-st, yup option 2 makes a lot of sense to me and feels intuitive, Also mostly people will be using default Address component without passing any size in most cases so looks good in that and its getting freaking messier and messier as we try to make it very very perfect but it looks great now I feel!! Lol yesterday in flight I was trying to solve it and came up with #942 but your solution looks more robust and works great!! Just pushed a commit updating the loading state when address is undefined inline with new styles. Before :Screen.Recording.2024-09-18.at.12.04.14.PM.movAfter :Screen.Recording.2024-09-18.at.11.52.51.AM.movcc @carletex for his final thoughts once! But looks good to me! |
Thanks all for the detailed display testing and exploring this! I really like how it looks 👍 |
Good point! Completely forgot about this state. Noticed that height of the component is different when loading and when address is loaded, but ens is still loading. So we have two jumps when loading in case we don't have ens: Screen.Recording.2024-09-18.at.12.52.32.movOr one when we have ens we have one jump: Screen.Recording.2024-09-18.at.12.51.24.movFirst thought is to make gap between items 1px, so first jump will not be so noticeable. But it looks not so good on large sizes. I didn't push this change, feel free to update if you think it's better than current. Screen.Recording.2024-09-18.at.13.05.57.movAlso, noticed that loading of components with Screen.Recording.2024-09-18.at.13.09.20.mov |
Fixed this Screen.Recording.2024-09-18.at.13.54.30.movPage code
|
Tysm @rin-st!! Make sense! |
Merging this 🙌 |
Description:
Fixes #915, but instead of keeping it default we enable it with
showBoth
prop (We can go with it being default too!, I am completely 50 50 on it so both seems good solution)Test code :