extension
extension copied to clipboard
Full page extension views, containers, shared headers + footers
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
withRadix.Dialog
and gather together all of our header code - [X] deprecate components like
ControlledDrawer
and simplify things migrating toDialog
s - [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
andnavigate
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
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 👍🏼
@pete-watters just tested this PR and it's broken the elastic scroll behaviour on macOS
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
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
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
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 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?
@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 nice job! Let me know if it's ready for design review :)
@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
Able to scroll to an odd position on unlock screen
Edit: hadn't refreshed service worker so window size was off
Oddly elongated pop up for sign in. Should it be this tall?
@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.
The padding off the text changes once the UI updates you're signing in
@pete-watters quick question, after this task, will the containers be a shared component that can be added to the design system and storybook?
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 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
Edit: hadn't refreshed service worker so window size was off Oddly elongated pop up for sign in. Should it be this tall?
![]()
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
Edit: hadn't refreshed service worker so window size was off Oddly elongated pop up for sign in. Should it be this tall?
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.
Edit: hadn't refreshed service worker so window size was off Oddly elongated pop up for sign in. Should it be this tall?
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.
I'm moving this back to DRAFT
as I have some more work to do implementing shared Header
s + refactoring pages with a two column layout (password, lock screen, key etc.)
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 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.
@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.
@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 myButton
andLink
s 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
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.
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
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.
Logo is bigger in wallet than designs
Page background changes when modal is open
Bg on homepage | With receive open |
---|---|