Skip to content
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

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

pete-watters
Copy link
Contributor

@pete-watters pete-watters commented Apr 5, 2024

Try out Leather build 724e604Extension build, Test report, Storybook, Chromatic

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

@@ -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;
Copy link
Contributor Author

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

Copy link
Collaborator

@kyranjamie kyranjamie left a 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? 🤔

@pete-watters
Copy link
Contributor Author

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

@pete-watters pete-watters added this pull request to the merge queue Apr 5, 2024
Merged via the queue into dev with commit 629ef97 Apr 5, 2024
28 of 29 checks passed
@pete-watters pete-watters deleted the fix/5149/virtuoso-partial branch April 5, 2024 13:35
@markmhendrickson
Copy link
Collaborator

Do we expect the following sort of report to be resolved by this PR?

Screenshot 2024-04-08 at 11 37 29

@pete-watters
Copy link
Contributor Author

Do we expect the following sort of report to be resolved by this PR?

Screenshot 2024-04-08 at 11 37 29

No, thats a separate problem. This fix relates to loading all of the accounts at once in the account selector and hadn't been released to production yet.

@markmhendrickson
Copy link
Collaborator

markmhendrickson commented Apr 9, 2024

@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?

@alter-eggo
Copy link
Contributor

@markmhendrickson there is skeleton loader now on dev instead of 0 balance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Virtual lists broken
4 participants