scaffold-eth / scaffold-eth-2

Open source forkable Ethereum dev stack
https://scaffoldeth.io
MIT License
1.41k stars 887 forks source link

address component showBoth prop #924

Closed technophile-04 closed 2 months ago

technophile-04 commented 2 months ago

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)

Screenshot 2024-09-04 at 10 55 49 PM (2)

Test code : ```tsx "use client"; import type { NextPage } from "next"; import { Address } from "~~/components/scaffold-eth"; const Home: NextPage = () => { const demoAddress = "0x34aA3F359A9D614239015126635CE7732c18fDF3"; return ( <>

Address Component Showcase

Without showBoth

Size: xs

Size: sm

Size: base(default)

Size: lg

Size: xl

Size: 2xl

Size: 3xl

With showBoth

Size: xs(default)

Size: sm

Size: base

Size: lg

Size: xl

Size: 2xl

Size: 3xl

); }; 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 ( <>

Address Component Showcase

Without showBoth

{sizes.map(size => (

Size: {size}

))}

With showBoth

{sizes.map(size => (

Size: {size}

))}
); }; export default Home; */ ```
technophile-04 commented 2 months ago

Also fixed the size of Copy Icon not being displayed properly in our original address component:

Main branch this branch
Screenshot 2024-09-04 at 10 59 34 PM Screenshot 2024-09-04 at 10 59 13 PM

Notice in this branch the size of copyIcon is responsive with respect to text size as compared to main branch

technophile-04 commented 2 months ago

Thanks Rinat and Damu for review! pushed some commits.

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

Yeah I wan't sure how best we could fix the layout shift, but I added currently the skeleton for now :

https://github.com/user-attachments/assets/c5c7fa2f-0c70-495c-804f-4b111afb988d

So 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)

rin-st commented 2 months ago

Good changes, thanks!

Updated a bit:

https://github.com/user-attachments/assets/67668bef-c255-4d6c-8cf3-6c179d9317bd

rin-st commented 2 months ago

Yeah I wan't sure how best we could fix the layout shift, but I added currently the skeleton for now : So 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)

I think it's a good solution! I thought about something like fixing height of component, but it will add empty space around, when showBoth is false. So I thing it's ok for now

technophile-04 commented 2 months ago

Updated a bit:

  • added 4xl size, so font-size on showBoth 3xl differs from showBoth 2xl
  • added possibility to use format="long" with showBoth
  • removed old useEffects which caused small jump

Thanks @rin-st makes sense!

carletex commented 2 months ago

This looks great @technophile-04 !! <3

The only thing that does not quite convince me is the showBoth prop. It's a bit vague. Not a big deal, but let me open some options:

IDK, just throwing it out there..... Happy to merge as it's too.

damianmarti commented 2 months ago

This looks great @technophile-04 !! <3

The only thing that does not quite convince me is the showBoth prop. It's a bit vague. Not a big deal, but let me open some options:

  • hideAddress - hideAddressIfEns / showAddress or showAddressIfEns: depending on the default.
  • full / compact: depending on the default. We already have the long / short so it might be a bit confusing (I've never used the long format btw)

IDK, just throwing it out there..... Happy to merge as it's too.

Maybe something more explicit like showAddressAndEns?

rin-st commented 2 months ago

This looks great @technophile-04 !! <3

The only thing that does not quite convince me is the showBoth prop. It's a bit vague. Not a big deal, but let me open some options:

  • hideAddress - hideAddressIfEns / showAddress or showAddressIfEns: depending on the default.
  • full / compact: depending on the default. We already have the long / short so it might be a bit confusing (I've never used the long format btw)

IDK, just throwing it out there..... Happy to merge as it's too.

hmm, in https://github.com/scaffold-eth/scaffold-eth-2/discussions/911 Austin said

if it has an ens, show that as the main info, but always show the address too (shortened by removing most middle characters)

So looks like we don't need all that props and showBoth too. Show both if we can and just address if no ens.

But we have format="long" in AddressQRCodeModal and TransactionComp so it will change

technophile-04 commented 2 months ago

Just pushed a changes removing showBoth prop and now using onlyEnsOrAddress. Hence now showing ENS + Address as default.

When you pass onlyEnsOrAddress we get the same behaviour as current main brach.

Here is the behaviour for default thing <Address address={demoAddress} />

  1. This will show all three things (loading skeleton as placeholder for ENS name while loading)
  2. If there is not ENS for that address then it fallbacks only blockie with address (same size as main branch)
Case 1 demo address as has ENS : https://github.com/user-attachments/assets/fe1d2181-fdbf-468a-a69b-9b6e77564c80
Case 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: `
`. But if you pass higher sizes then layout shift is more visible since when 3 three things are shown we want to scale blockie to 4th step and when no ens is present we want to decrease it size. https://github.com/user-attachments/assets/70f99e7c-0203-40d7-9d6f-883a29e89be8
here is example test code : ```tsx "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 ( <>

Address Component Showcase

Only Address or ENS

Size: xs

Size: sm

Size: base(default)

Size: lg

Size: xl

Size: 2xl

Size: 3xl

Default Address Behaviour

Size: xs(default)

Size: sm

Size: base

Size: lg

Size: xl

Size: 2xl

Size: 3xl

); }; 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 ( <>

Address Component Showcase

Without showBoth

{sizes.map(size => (

Size: {size}

))}

With showBoth

{sizes.map(size => (

Size: {size}

))}
); }; export default Home; */ ```
rin-st commented 2 months ago

Great, thanks!

One more nit: after 8df4dbc Address component without size prop (default) has different font-sizes for address when loading and when loaded, but if size is passed it has same font-size. As I understand we need to decrease font-size everywhere when loading or not decrease at all

https://github.com/user-attachments/assets/d2763f6a-b812-49e8-86ff-820fe96dad5e

rin-st commented 2 months ago

One more nit: after 8df4dbc Address component without size prop (default) has different font-sizes for address when loading and when loaded, but if size is passed it has same font-size. As I understand we need to decrease font-size everywhere when loading or not decrease at all

I played with it (a lot) and as I understand we have two options.

  1. Use the same address font size with onlyEnsOrAddress prop and without it.
    • Pros: Readable text
    • Cons: Big layout shift, big height difference between components with the same size but with different onlyEnsOrAddress prop

https://github.com/user-attachments/assets/d5277d95-e34a-4bf9-84e3-73bc71a2383e

  1. Use smaller address size without onlyEnsOrAddress or when loading if no ens.
    • Pros: Smaller layout shift; smaller height difference between components with the same size but with different onlyEnsOrAddress prop, so it looks better on loading
    • Cons: small font-size on size="xs", and even size="sm" when showing both address and ens(or skeleton). Notable layout shift on size="xs" and size="sm"

https://github.com/user-attachments/assets/5594adee-d840-4654-a64b-af14a45f4884

Both cases uses default size="base", so default component behaves the same as with size="base"

Pushed the second option since I think it looks better but feel free to update/revert if needed

Page code (3 colons)

``` "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 demoAddressEns = "0x34aA3F359A9D614239015126635CE7732c18fDF3"; return ( <>

Address Component Showcase

{/*
Size is default, onlyEnsOrAddress
Size is default, no onlyEnsOrAddress
*/}

Only Address or ENS

Size: xs

Size: sm

Size: base(default)

Size: lg

Size: xl

Size: 2xl

Size: 3xl

Default Address Behaviour

Size: xs

Size: sm

Size: base (default)

Size: lg

Size: xl

Size: 2xl

Size: 3xl

Default Address Behaviour (with ens)

Size: xs

Size: sm

Size: base (default)

Size: lg

Size: xl

Size: 2xl

Size: 3xl

); }; 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 ( <>

Address Component Showcase

Without showBoth

{sizes.map(size => (

Size: {size}

))}

With showBoth

{sizes.map(size => (

Size: {size}

))}
); }; export default Home; */ ```

technophile-04 commented 2 months ago

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 https://github.com/scaffold-eth/scaffold-eth-2/pull/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 : https://github.com/user-attachments/assets/cc6643c5-85ec-47a3-b429-8647cbcf82b9
After : https://github.com/user-attachments/assets/d59b31ce-e3fd-456a-b24d-d4a35db1bcf3

cc @carletex for his final thoughts once! But looks good to me!

carletex commented 2 months ago

Thanks all for the detailed display testing and exploring this!

I really like how it looks :+1:

rin-st commented 2 months ago

Just pushed a commit updating the loading state when address is undefined inline with new styles.

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:

https://github.com/user-attachments/assets/f83621ff-4e7a-4643-915e-5a24fc7c6224

Or one when we have ens we have one jump:

https://github.com/user-attachments/assets/e61d4801-7db0-444b-880e-780141534df0

First 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.

https://github.com/user-attachments/assets/17dd88e2-0577-492c-9ebd-dfaa4c5d670a

Also, noticed that loading of components with onlyEnsOrAddress has a larger height

https://github.com/user-attachments/assets/ed686b01-9004-4817-8615-939ca52697cb

rin-st commented 2 months ago

Also, noticed that loading of components with onlyEnsOrAddress has a larger height

Fixed this

https://github.com/user-attachments/assets/e021c3b8-ff62-4ef8-a94e-039786e02b28

Page code

``` "use client"; import { useEffect, useState } from "react"; import type { NextPage } from "next"; import { Address } from "~~/components/scaffold-eth"; const Home: NextPage = () => { const [demoAddress, setDemoAddress] = useState(undefined); const [demoAddressEns, setDemoAddressEns] = useState(undefined); useEffect(() => { const timer = setTimeout(() => { setDemoAddress("0x4e123738A66427ee9BD0C9dc67C09af397A7D93e"); setDemoAddressEns("0x34aA3F359A9D614239015126635CE7732c18fDF3"); }, 2_000); return () => clearTimeout(timer); }, []); return ( <>

Address Component Showcase

Only Address or ENS

Size: xs

Size: sm

Size: base(default)

Size: lg

Size: xl

Size: 2xl

Size: 3xl

Default Address Behaviour

Size: xs

Size: sm

Size: base (default)

Size: lg

Size: xl

Size: 2xl

Size: 3xl

Default Address Behaviour (with ens)

Size: xs

Size: sm

Size: base (default)

Size: lg

Size: xl

Size: 2xl

Size: 3xl

); }; 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 ( <>

Address Component Showcase

Without showBoth

{sizes.map(size => (

Size: {size}

))}

With showBoth

{sizes.map(size => (

Size: {size}

))}
); }; export default Home; */ ```

technophile-04 commented 2 months ago

Tysm @rin-st!! Make sense!

technophile-04 commented 2 months ago

Merging this 🙌