Closed pete-watters closed 5 months ago
@markmhendrickson : we had a call yesterday to clear up the scope of #4165 . We have split up the work into two issues #4370 and #4371
CC @mica000 @kyranjamie @fabric-8
This issue needs to also fix this: https://github.com/leather-wallet/extension/issues/4477
Related to issue https://github.com/leather-wallet/extension/issues/4477 this issue also needs to resolve https://github.com/leather-wallet/extension/issues/4549
When working on this we can also add our own custom scroll bars and fix the other issues listed in https://github.com/leather-wallet/extension/issues/3488
We had another report of issues with the account select list we need to solve here https://github.com/leather-wallet/extension/issues/4568
I presume the Ledger-specific modals will also be covered by this general update?
I presume the Ledger-specific modals will also be covered by this general update?
We will have to do it for all modals so they look OK in pop-up mode so we will include Ledger also.
In your screenshot, that's actually in pop-up mode, so I guess we should also include it there too
I've been working on this as part of this PR. In order to create containers, it first makes sense to fix this issue of modals / full page views
@markmhendrickson : I've opened a PR with this work done. I added a demo video you can see here
I've made some good progress with this but there are still some bugs I need to fix in the PR.
I'll fix those and then get to work creating re-usable headers / footers
Another fix to be included here is https://github.com/leather-wallet/extension/issues/4714
We had an external contributor open this PR which adds a workaround but the issue is more to do with the screen height not responding properly so I will take it into consideration for containers and fix it properly.
There's some changes to make to the onboarding screens that affect the headers so I will tackle those also and do everything together.
I'm still working on this but giving a progress update. This week I've:
action popout
bg black regardless of themeHeader
+ Footer
components in Storybook Footer
s in the extension for the new UI Library versionNext up to move on with this is to:
extension
to use the new Header
component
Header
that rely on context - navigation + networkHeader
s with the UI libraryHere's a demo video explaining a bit more:
https://github.com/leather-wallet/extension/assets/2938440/a686cab6-d04e-42d2-b4e9-40a7dec17e4c
A MA ZING! Literally clapping over here. Also great demo! Thanks Pete!
About the Ledger hovering state - so it currently only happens when you hover the logo?
A MA ZING! Literally clapping over here. Also great demo! Thanks Pete!
About the Ledger hovering state - so it currently only happens when you hover the logo?
Thanks! My first attempt using Screen Studio. Thanks to Fab for the recommendation.
Yeah I found some code in the ledger dialogs that was showing this message but only on hover. I'm not sure if it's behaviour we still need. I'll find out when setting up the header there.
Nice work, @pete-watters! The demo is fantastic – thanks for pulling it together.
Also I do recommend we remove that Ledger-specific message: https://github.com/leather-wallet/extension/issues/4689
Nice work, @pete-watters! The demo is fantastic – thanks for pulling it together.
Also I do recommend we remove that Ledger-specific message: #4689
Thanks @markmhendrickson 🙇
I'll get rid of the Ledger message so. That simplifies things and will help make the headers more generic
I have been hard at work on this again this week but it will still take some more time to complete. I estimate one more week, maybe even two to have it well tested and reviewed. As I work on this I have been encountering and fixing technical debt issues along the way.
This week I:
DropDown
for the settings menuHeader
component
jotai
state management on a per page basis <NetworkModeBade
so it's set at a higher level home
page has a background colour of accent.background-primary
page
pages (other non onboarding pages) have a background colour of accent.background-secondary
The next work I need to do is to:
popout
pages which are still using a legacy PopupHeader
account
display also
storybook
so we can visualise each pageGreat to see it's coming along! the hark work will pay off tons in the future!
I added a progress update of last weeks work here
I am working on pop-out headers now and documenting the decisions I make here. Please let me know if I miss something or am mistaken.
Of the extension headers we have (screenshot here), I am organising into these types.
These are shown in dialogs / cards inside the APP
These are shown in popout windows triggered by interactions
I checked the code and we have a few different types of these popouts for different reasons. We use different paths to signify their intention. The paths I found are, based on the Test APP are:
#/update-profile
#/signature
#/psbt
#/transaction
#/get-addresses
Some of these may be outdated or I may have missed some but this is a good start and I will apply the following headers for each. I have provided a screenshot of the old and new behaviour of these popouts. note: The new is WIP so not yet correct
Will use this new header:
Current OLD and NEW
Will use this header but with no action menu unless desired
Current OLD and NEW
Will use this header:
Current OLD and NEW
Will use this header:
Current OLD and NEW
Will have no header as per previous design, unless desired
Of the above, it's unclear to me if some of these should instead use these other headers:
@fabric-8 @mica000 @markmhendrickson: My plan is to implement as per detail above but if this is incorrect please let me know so I can make adjustments
Thanks a lot for all the work and the super clear update @pete-watters ! Guess I'll be blocking out a day soon to fully test the update once it's ready 😅
I presume the Ledger-specific modals will also be covered by this general update?
I've been checking this @markmhendrickson and I have improved the dialog the ledger notifications show up in but I noticed a lot of them already have padding issues in production.
It might be prudent to fix those screens also but maybe in a subsequent PR once the main one is merged.
Designs for Receive Asset, Receive collectible, Choose asset to send
What we need:
This is the new recipe for the existing BitTitle Dialog (Maybe we want to call it smt else now that it wont have a BigTile? For now naming it EmptyDialoag;). As you can see the Action pop-up will have the header always present, but only showing the arrow back.
Steps that need this:
@pete-watters keeping the padding 24px;
This is how they are build:
We need a logic that, when on Action Pop-up and on FullPages, hides the close button. when on EmptyDialog, shows close button.
Sounds good @mica000, thanks!
@markmhendrickson This type of thing is why I am pushing for written specs / graphs, something. We need to consider how user flows link together when creating designs. Not just for development, but so we have a clear plan of what we are doing.
This issue of inconsistent flows isn't new, it's actually how Leather already works so we could have planned for this change originally by deciding how we wanted it to work and avoided so much of this re-work and changes
When working on https://github.com/leather-wallet/extension/issues/4165 it became apparent that more changes would be needed to support the new design.
It's desired that for most screens:
This task relates to #4370 and builds upon #4165
Add resource in designs