-
Notifications
You must be signed in to change notification settings - Fork 145
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
Full page extension views, containers, shared headers + footers #4655
Conversation
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 |
2023-12-07-000038.mp4This 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 |
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 |
08e150b
to
bbf8288
Compare
We tried a few things and went with this. TLDR - Extension + popup are the same dimensions now - |
@@ -17,12 +16,14 @@ export function HighFeeDrawer(props: { learnMoreUrl: string }) { | |||
}, [isShowingHighFeeConfirmation, setIsShowingHighFeeConfirmation]); | |||
|
|||
return ( | |||
<ControlledDrawer | |||
icon={<ErrorIcon color="error.label" size="xl" />} | |||
<BaseDrawer |
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.
We pass an ErrorIcon
to this. I removed it as this was the only place using it.
I'm changing it so the warning icon will appear in the modal beside the text:
NOTE - I forced display of this so it's missing the amount Are you sure you want to pay AMOUNT in
What do you think @mica000 @fabric-8 - is it OK to move this icon from the header? It's the only place we have one there.
I will move the close icon to the right also
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.
Good with me!
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, thanks! It's coming soon(tm) 🚀
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.
@markmhendrickson this is the High Fee
dialog I mentioned to you yesterday.
@fbwoolf - do you know how to test this properly / how it should work. I tried to refactor it as it was similar to other dialogs, storing open / close in jotai
state. I added new simpler useState
handlers but I need to make sure it works.
You can search for setIsShowingHighFeeConfirmation
on my branch
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.
@kyranjamie - see this RE HighFee dialog.
I guess it shows when the fee is considered high but I am lacking knowledge here as to what is considered a high fee.
e.g. as a user when I try and transfer X stacks and the fee is Y then show the high fee warning
I think it's still working after my changes but I'm not certain so I want to check
0df2194
to
4ce1884
Compare
5e84a1a
to
6223fd1
Compare
src/app/ui/components/containers/modal/components/radix-dialog.tsx
Outdated
Show resolved
Hide resolved
width: { base: '100vw', md: '90vw' }, | ||
height: { base: '100vh', md: 'unset' }, | ||
maxWidth: { base: '100vw', md: '450px' }, | ||
maxHeight: { base: '100vh', md: '85vh' }, |
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.
I'm using the panda
style of responsive objects as when I was using our hooks useViewportMinWidth
I was noticing some extra re-renders and jumpy behaviour.
For example on the Recieve
modal, the UI was re-rendering and flashing when moving from Receive
to Receive BTC
b9effeb
to
733498b
Compare
74e848e
to
ebb1742
Compare
I worked through the remaining bugfixes and changes in Notion and as discussed. I have fixed them all apart from outstanding work to be decided on background colours globally. There are a few issues in Notion I am now unable to reproduce:
I changed the signature headers, hopefully getting them all but I'm not sure @fbwoolf @kyranjamie if you can please review the latest code changes that would be great. Then I can hopefully add one last commit that updates the colours. I'm not sure if we do need to work on the receive screens also as we always need to scroll to see the address for STX / receive ordinal Area.mp4 |
ebb1742
to
927ee6f
Compare
5f31574
to
1e845d1
Compare
|
||
import { isKnownPopupRoute } from './get-popup-header'; | ||
|
||
function isHomePage(pathname: RouteUrls) { |
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.
@pete-watters I was thinking about this implementation yesterday where you are using variants of one container with lots of detecting of routes, I am wondering what you think about an alt approach where we just have several, more specific, containers that wrap the routes that need it? I know the code might be less DRY, but sometimes it makes scaling the code a bit easier long term? Same question for headers?
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.
I agree, its a real mess. A lot of this complication came from having different background colours for different page types e.g. Home
and Page
. It was also a bridge to get us away from how we were doing things before - storing Header
components in application state.
We are getting rid of the colour variations now and having more normalised page backgrounds so when I am working on that I will try and simplify everything and ditch these complex variants.
Once everything is working and signed off we can assess and figure out a better pattern to use
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.
Got it, yep, makes sense. 👍
c2f1ec3
to
429c842
Compare
@fabric-8 @markmhendrickson @mica000 : I have updated the colour styles to match the new design. Please take a look and let me know if there are any more updates to make. We can do a further QA now and hopefully sign off on this. @kyranjamie @fbwoolf : I squashed all the commits again. This should be code complete now bar any further updates so I'd appreciate on final review. I will think about how to implement a better software architecture design to simplify things now we have made the final decision on colours. I'll try to remove some of the spaghetti code looking up routes etc. and use more container wrappers as Fara suggested |
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.
Huge effort, Pete. Let's get it MERGED
…nboarding, settings, ref #4371
a0eeb00
to
0f33d98
Compare
This PR covers:
These changes also fix a few bugs:
There were several steps involved and lots of refactoring:
baseDrawer
withRadix.Dialog
and gather together all of our header codeControlledDrawer
and simplify things migrating toDialog
snavigate
out of Dialogstate
andnavigate
hooks