-
Notifications
You must be signed in to change notification settings - Fork 149
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
chore: implement fix to limit amount of accounts rendered #5170
Conversation
724e604
to
c71c2cf
Compare
@@ -43,6 +43,7 @@ export const SwitchAccountDialog = memo(({ isShowing, onClose }: DialogProps) => | |||
if (!isShowing) return null; | |||
|
|||
const accountNum = stacksAddressesNum || btcAddressesNum; | |||
const maxAccountsShown = accountNum > 10 ? 10 : accountNum; |
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 - you were right about this and I had naively set the list to have a generated height based on all of the accounts. This change limits it to only allow the height of 10 accounts to be rendered.
When I test I still see more than 10 rendered so I need to figure that out. I see 15
items rendered when I can only see 9 in the viewport.
The reason I changed this code was so that the UI could smoothly adjust and increase the list height on smaller width screens. Adding this code impacts that but I am close to having a better fix in: #5171
I just thought it best to get something mergeable for this as it was a bad bug on dev
and a release blocker
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.
Thanks Pete
They do claim to support dynamic window heights, so wonder what the issue is here. What if we don't set a height at all? 🤔
If we don't set the height it just renders an empty list so we don't see anything. I'm close to having something good and auto sizing in https://github.com/leather-wallet/extension/pull/5171/files , I just need to test it a bit better. I will check if there is a cleaner way with dynamic heights on their docs |
@alter-eggo are you working on this other problem (see comment above) already via other issues, or do we need to capture as a new issue? |
@markmhendrickson there is skeleton loader now on dev instead of 0 balance |
This PR makes a small change to fix #5149.
This fixes the issue and reduces the impact of the bug however it means that the UI isn't as slick as it could be and the list height doesn't dynamically update on narrower viewports.
I am close to having a full solution in this PR but thought it worth improving what we have on
dev
while I am finalising that.Here's what I am talking about:
Area.mp4