extension icon indicating copy to clipboard operation
extension copied to clipboard

Full page extension views, containers, shared headers + footers

Open pete-watters opened this issue 1 year ago • 57 comments

Try out this version of Leather — Extension build, Test report

This PR covers:

These changes also fix a few bugs:

  • https://github.com/leather-wallet/extension/issues/4692
  • https://github.com/leather-wallet/extension/issues/4250

There are several steps involved:

  • [X] replace baseDrawer with Radix.Dialog and gather together all of our header code
  • [X] deprecate components like ControlledDrawer and simplify things migrating to Dialogs
  • [X] standardise the viewport width we use so that extension mode matches popout
  • [X] move our CSS reliance (radix, global styles) from APP -> UI library
  • [X] make our modals look like full pages in extension view
  • [X] add new UI components to story book
  • [X] refactor uses of navigate our of Dialog
  • [X] replace all headers with new components - refactoring to remove reliance on state and navigate hooks
  • [X] implement screen background colour change based on screen variant ('home', 'page' etc.)
  • [X] finish work on new set-password page + onboarding pages
  • [X] finish work on two-column layout for mnemonic keys
  • [X] add new settings dropdown https://github.com/leather-wallet/extension/issues/4313
  • [X] https://github.com/leather-wallet/extension/issues/4826
    • [X] https://github.com/leather-wallet/extension/issues/4783
    • [X] https://github.com/leather-wallet/extension/issues/4826

Remaining items to get this over the line are:

  • [] test scroll behaviour app wide
  • [] fix bugs with popup content BG colours
  • [] fix settings menu sub-menu + add new icons
  • [] test on windows

Demo 1 (Dec 2023): You can see a video of the changes here, including:

  • my standard wallet
  • wallet with lots of accounts
  • wallet in ledger mode

https://github.com/leather-wallet/extension/assets/2938440/a6ad7af1-e872-4738-b07b-8900c03e7cb6

Demo 2 (Jan 2024): You can see an updated demo here

pete-watters avatar Dec 07 '23 11:12 pete-watters

Any reason we don't make the extension popup bigger instead of the other one smaller? If we can use one html file for both then 👍🏼

kyranjamie avatar Dec 07 '23 12:12 kyranjamie

@pete-watters just tested this PR and it's broken the elastic scroll behaviour on macOS

kyranjamie avatar Dec 07 '23 12:12 kyranjamie

dev

https://github.com/leather-wallet/extension/assets/1618764/2e168679-085f-4378-b474-6a67b87535f8

This PR

https://github.com/leather-wallet/extension/assets/1618764/602d1e78-f343-412f-8e79-26c0023dc9af

kyranjamie avatar Dec 07 '23 12:12 kyranjamie

dev

2023-12-07-000038.mp4 This PR

2023-12-07-000039.mp4

Thanks, I didn't know we had that or how rich you are! I'll make sure to fix it

pete-watters avatar Dec 07 '23 14:12 pete-watters

Any reason we don't make the extension popup bigger instead of the other one smaller? If we can use one html file for both then 👍🏼

I'll check and try make it bigger. I found how we specify the pop-out size more easily than how we specify the extension size but I'll investigate. Thanks!

One reason to make it smaller is @mica000 said we use 392px in the designs for extension size. I will work with her to discuss making it bigger

pete-watters avatar Dec 07 '23 14:12 pete-watters

Any reason we don't make the extension popup bigger instead of the other one smaller? If we can use one html file for both then 👍🏼

We tried a few things and went with this.

TLDR - Extension + popup are the same dimensions now - 390 x 756 . We can assess that and tweak as required

pete-watters avatar Dec 14 '23 09:12 pete-watters

@pete-watters thanks for all the work here, I'll download the build and test today. It looks like in the video the background shifts when the modal is launched?

fbwoolf avatar Dec 15 '23 15:12 fbwoolf

@pete-watters thanks for all the work here, I'll download the build and test today. It looks like in the video the background shifts when the modal is launched?

Thanks @fbwoolf . Let me check that out. There's probably a few weird things like that I missed

pete-watters avatar Dec 18 '23 05:12 pete-watters

@pete-watters nice job! Let me know if it's ready for design review :)

mica000 avatar Dec 18 '23 14:12 mica000

@pete-watters nice job! Let me know if it's ready for design review :)

Thanks @mica000 😄 If you have time for a quick review now that would be great. You can wait a bit longer if you want also.

This solves a lot of the problems we had in extension mode but I didn't actually setup the shared headers yet

pete-watters avatar Dec 18 '23 15:12 pete-watters

Able to scroll to an odd position on unlock screen

image

kyranjamie avatar Dec 19 '23 11:12 kyranjamie

Edit: hadn't refreshed service worker so window size was off image

Oddly elongated pop up for sign in. Should it be this tall?

image

kyranjamie avatar Dec 19 '23 11:12 kyranjamie

@pete-watters nice job! Let me know if it's ready for design review :)

Thanks @mica000 😄 If you have time for a quick review now that would be great. You can wait a bit longer if you want also.

This solves a lot of the problems we had in extension mode but I didn't actually setup the shared headers yet

Awesome, the things I see are related to 'headers' and 'footers' positions which as your mentioned aren't covered in this build yet.

mica000 avatar Dec 19 '23 11:12 mica000

The padding off the text changes once the UI updates you're signing in

image

kyranjamie avatar Dec 19 '23 11:12 kyranjamie

@pete-watters quick question, after this task, will the containers be a shared component that can be added to the design system and storybook?

mica000 avatar Dec 19 '23 11:12 mica000

I found a bug here in the new modal regarding re-sizing of height not working in small viewports. Working on a fix now as that was mostly working previously

https://github.com/leather-wallet/extension/assets/2938440/ed7682cf-059c-4abf-a690-96838f0b9229

pete-watters avatar Dec 19 '23 11:12 pete-watters

@pete-watters quick question, after this task, will the containers be a shared component that can be added to the design system and storybook?

Yes my plan is to build them as independent components. I'm adding them under our ui folder which will be the new design system and eventually storybook.

That will take a bit more work but thought it good to get some eyes on this as I thought I'd be breaking a few things here

pete-watters avatar Dec 19 '23 12:12 pete-watters

Edit: hadn't refreshed service worker so window size was off Oddly elongated pop up for sign in. Should it be this tall?

image

It was intentional but I agree it looks a bit too big for this screen. What do you think @mica000 ?

Having this extra height does make it more consistent with mobile and we can show more things there

pete-watters avatar Dec 19 '23 12:12 pete-watters

Edit: hadn't refreshed service worker so window size was off Oddly elongated pop up for sign in. Should it be this tall? image

It was intentional but I agree it looks a bit too big for this screen. What do you think @mica000 ?

Having this extra height does make it more consistent with mobile and we can show more things there

@pete-watters @kyranjamie My understanding was that this PR wasn’t solving anything related to footer and headers yet, is that still the case? Otherwise if not, then yes, there are a few more issues with footers that needs to be addressed as they should always stick to the bottom.

Curious on your take around how to orchestrate this but think this PR should only be released once we have the three components in place (container, footers and headers)?

And I'm assuming that since these are introducing a new frame to each screen, we will probably need resolve the padding issues for each case.

mica000 avatar Dec 19 '23 17:12 mica000

Edit: hadn't refreshed service worker so window size was off Oddly elongated pop up for sign in. Should it be this tall? image

It was intentional but I agree it looks a bit too big for this screen. What do you think @mica000 ? Having this extra height does make it more consistent with mobile and we can show more things there

@pete-watters @kyranjamie My understanding was that this PR wasn’t solving anything related to footer and headers yet, is that still the case? Otherwise if not, then yes, there are a few more issues with footers that needs to be addressed as they should always stick to the bottom.

Curious on your take around how to orchestrate this but think this PR should only be released once we have the three components in place (container, footers and headers)?

And I'm assuming that since these are introducing a new frame to each screen, we will probably need resolve the padding issues for each case.

There is more work to come with footers indeed. I'll make the footers sticky at the bottom and then we can assess if it's still too big. Thanks!

As mentioned I don't think I will merge this until all the work is done, I was just seeking interim feedback to help get phase 1 into a good state. I'll sort out the header/ footer stuff then we can check again.

pete-watters avatar Dec 20 '23 10:12 pete-watters

I'm moving this back to DRAFT as I have some more work to do implementing shared Headers + refactoring pages with a two column layout (password, lock screen, key etc.)

pete-watters avatar Dec 21 '23 09:12 pete-watters

I'm going to close this off for a bit. We have some changes to make to the onboarding screens that affect the headers so I will tackle those also and do everything together.

pete-watters avatar Dec 22 '23 16:12 pete-watters

@pete-watters looked this over and code looks good so far. I'm not sure where to jump in yet with actual CR just bc there seems to be a lot still in-flight. If you want to squash your commits into one, I can def rebase this for you on my changes if helps. Sorry for all the conflicts.

fbwoolf avatar Jan 20 '24 17:01 fbwoolf

@pete-watters looked this over and code looks good so far. I'm not sure where to jump in yet with actual CR just bc there seems to be a lot still in-flight. If you want to squash your commits into one, I can def rebase this for you on my changes if helps. Sorry for all the conflicts.

Thanks Fara! I'll sort the conflicts don't worry. I'll rebase again soon, I've been keeping progress commits as I go.

pete-watters avatar Jan 22 '24 06:01 pete-watters

@kyranjamie @fbwoolf : I'm opening this up for review so please provide feedback when you can.

I still have some outstanding tasks to solve but thought I better start the review in case there is some things that need to be fixed / some code changed.

With the deprecation of useRouteHeaderState I had to perform some logic based on route switches called from container.tsx. I also started to get rid of some other jotai state that was used to store if / when various dialogs were open. I hope that's OK? We could be close to removing that library if we wanted.

The main outstanding work I have to do is:

  • task #1 - fix virtuoso overflow (thought I had done this but on fresh install its not working)
  • task #2 - re-test dialogs, LEDGER DIALOGS + all popouts
  • task #3 - finish settings menu, add icons, get rid of sub-menu in favour of modal
  • task #4 - cross platform tests (on Windows etc. )
  • task #5 - get the playwright tests passing
  • task #6 - clean up my new storybook stories
  • task #7 - finalise and clean the code - remove TODOs, rebase etc. container.tsx for example is still messy
  • task #8 - figure out why all my Button and Links have weird colours

I hope I'm not jumping the gun opening now but I likely won't get all these tasks done today but thought it best to start the review. I'll record a proper demo soon and open it up for design review then also

pete-watters avatar Feb 14 '24 14:02 pete-watters

Great work here @pete-watters, will review more tom. I got held up today trying to open the icons PR and hitting some pain points with panda. Hoping to get that merged so you can use the new icons in the settings menu.

fbwoolf avatar Feb 15 '24 02:02 fbwoolf

Great work here @pete-watters, will review more tom. I got held up today trying to open the icons PR and hitting some pain points with panda. Hoping to get that merged so you can use the new icons in the settings menu.

No probs at all. Plenty more updates to be done here so another day helps me

pete-watters avatar Feb 15 '24 03:02 pete-watters

Reviewing now, will leave each piece of feedback in a new comment

Alignment on home screen seem off. Shouldn't the Leather align on left with content? Also the version number no longer aligns on the baseline of Leather logo.

image

kyranjamie avatar Feb 15 '24 09:02 kyranjamie

Logo is bigger in wallet than designs

image image

kyranjamie avatar Feb 15 '24 09:02 kyranjamie

Page background changes when modal is open

Bg on homepage With receive open
image image

kyranjamie avatar Feb 15 '24 09:02 kyranjamie