fix: allow users access all accounts in account switcher

[pete-watters]

Try out Leather build 81da367 — Extension build, Test report, Storybook, Chromatic

[kyranjamie]

This function is named/located as thought it's a generic virtuoso list height function. But it isn't, because the first argument is specific to the accounts list, one particular instance of it.

Isn't this a coupling of unrelated concerns?

[pete-watters]

This is still inflight but when needed I can refactor accountNum to be data array.

We use Virtuoso in 3 areas and all of them relate to rendering of accounts lists

[kyranjamie]

We use Virtuoso in 3 areas and all of them relate to rendering of accounts lists

If you'll allow me to rebuttal this: the fact that the virtual lists we use currently relate to account lists, isn't reason enough to presume all future virtual lists are going to relate to account lists.

This code (if only conceptually) couples together a generic virtual list function with a specific type of data (accounts). I think that's a dangerous assumption, and one that breaks the single responsibility principal.

I imagine this situation: another developer is tasked with building an expensive list to render. They need to use this virtuoso function to get the height, and end up being hampered/confused by the fact there's account-specific in there, that they don't need or want.

What are your thoughts on this?

[pete-watters]

Perhaps I didn't communicate effectively previously but I will be changing this code and updating the variable names. The work is not finished yet and I'm unsure if this code will be needed, ideally not. That's why I posted it as a draft. I am trying to first fix the bug, then refactor it to be better.

I agree totally with you and I don't want to break the single responsibility principle. It doesn't make sense to couple this to accounts at all. At this point its just the name I was using as the data is accounts and I am focusing on the UI bugs.

Once thats fixed I will update the code to make it clean and reusable for the future

[kyranjamie]

Virtuoso is supposed to support dynamic heights. Did you figure out doing this why it doesn't work? Is it specific to the dialog component? It'd be great if this worked, and we didn't need a complicated height-setting hook

[pete-watters]

Virtuoso is supposed to support dynamic heights. Did you figure out doing this why it doesn't work? Is it specific to the dialog component? It'd be great if this worked, and we didn't need a complicated height-setting hook

It would be definitely better to not need this at all. I'll keep trying using their docs. In their examples they give Virtuoso a height of 100% but it has a wrapping div with a fixed height.

It has been complex as we want a want a dynamic height Virtuoso list inside of a dialog which then also has a header and sometimes a footer. I can get it mostly OK but when resizing the browser to be less tall we often have accounts spilling out of the dialog. When I fix that with a margin it tends to then result in not being able to see some account entries.

We have always seemed to have this issue with users either not being able to select the last account or else accounts in the list spilling out of the dialog.

Here's an idea of some of the problems:
https://github.com/leather-wallet/extension/assets/2938440/f4ba5d6c-eab4-4a02-9349-4bd64b84a5dc

Ideally we would have no need for a hook at all and it would just:

• resize properly and allow user to select the last account when there is a footer
• not spill out of the dialog when we reduce the height
• not render too big of a list with a scroll when there are less accounts
• behave properly when the window width is reduced and not have a huge gap to the footer