fix: allow virtuoso to resize dynamically, ref #5242 #5289

pete-watters · 2024-04-22T10:40:43Z

Try out Leather build 6f242c3 — Extension build, Test report, Storybook, Chromatic

This PR ensures that account lists grow appropriately and that users with many accounts can access all of them.

pete-watters · 2024-04-23T10:16:04Z

src/app/pages/choose-account/choose-account.tsx

@@ -34,12 +37,16 @@ export const ChooseAccount = memo(() => {
          {url && <RequesterFlag requester={url.toString()} />}
          <LogomarkIcon width="248px" height="58px" />
          <Stack gap="space.04">
-            <styled.h1 textStyle="heading.05">Choose an account to connect</styled.h1>
+            <styled.h1 textStyle="heading.05">


I added this code as if you only have BTC connected on Ledger and then try to sign in to something it was just showing the logo and an empty screen

Now it shows:

I believe we prob need some more explanation of what's going on for users?
something like 'you are connected with ledger in bitcoin mode only, please connect stacks account as well to use this feature' or smth like that

@mica000 @markmhendrickson @fabric-8 :

Do you have any input on what copy to use on this screen?

Right now :

AS A USER signed in with ledger

AND only have BTC connected

WHEN you try and sign in to a site e.g. Gamma

THEN you see an empty screen below the leather logo like this

I've just made a small change so that in these cases we say No connected accounts found instead of Choose an account to connect

Following up here a bit late:

Ideally the user should be able to connect their wallet to any site with a Ledger account, regardless of whether they've connected with Ledger for BTC or STX.

It seems that our legacy endpoint for connecting (used by Gamma and others) is not behaving this way. However, because it's legacy (and deprecated), we don't need to update it per se.

I also assume that our newer endpoint (getAddresses) does behave this way, though I haven't tested per se. And as we implement this endpoint for mobile, we should retain this agnostic behavior, allowing users to connect regardless of Ledger connectivity by token.

Thanks for following up anyway 👍

pete-watters · 2024-04-23T10:39:49Z

src/app/ui/components/virtuoso.tsx

+  const heightOffset = headerHeight + footerHeight;
+  const height = vhToPixels(virtualHeight) - heightOffset;
+
+  const onResize = () => {


I tried to be more precise and update to the exact height we wanted on re-size but it was buggy.

Instead, here I force a re-render on re-size by changing the key. This means the wrapper height can adjust properly according to the parents vh

pete-watters · 2024-04-23T10:40:54Z

src/app/ui/components/virtuoso.tsx

+}
+
+function vhToPixels(vh: string) {
+  const numericHeight = +vh.replace('vh', '');


This is converting vh to pixels so that it's a numeric value and we can compute the height of the wrapper, taking into account offsets needed for headers + footers

pete-watters · 2024-04-23T10:42:40Z

src/app/ui/components/virtuoso.tsx

+  return (numericHeight * window.innerHeight) / 100;
+}
+
+export function VirtuosoWrapper({ children, hasFooter, isPopup }: VirtuosoWrapper) {


I should improve this more by:

accepting the height as a prop

using tokens for footer / header height

I couldn't get them working using token('size

When importing and using token.size.X.value it was giving type errors

I can improve this further but I think this needs to get released sooner

pete-watters · 2024-04-23T10:43:39Z

src/app/ui/components/containers/dialog/dialog.tsx

-            >
-              {children}
-            </Box>
+            {wrapChildren ? (


All other dialogs have this Box to help them size dynamically but ones containing Virtuoso lists now bypass that.

pete-watters · 2024-04-23T10:44:32Z

...nd/send-crypto-asset-form/components/recipient-accounts-dialog/recipient-accounts-dialog.tsx

+      onClose={onGoBack}
+      wrapChildren={false}
+    >
+      <VirtuosoWrapper>


Virtuoso height is now set to 100% and the actual height is controlled by a wrapping container

pete-watters · 2024-04-23T10:46:42Z

src/app/pages/home/home.tsx

  const { decodedAuthRequest } = useOnboardingState();
-
+  const { isShowingSwitchAccount, setIsShowingSwitchAccount } =


I was investigating why we were re-rendering SwitchAccount and I found this mistake.

We were rendering it both inside of the main <Container and also inside of AccountCard.

Now it stays at Container level only and OutletContext is used to show / hide it.

It needs to stay at that level so the settings menu can also trigger it

pete-watters · 2024-04-23T10:47:06Z

src/app/features/message-signer/message-preview-box.tsx

@@ -17,8 +17,8 @@ export function MessagePreviewBox({ message, hash }: MessageBoxProps) {
        py="space.05"
        overflowX="auto"
      >
-        {message.split(/\r?\n/).map(line => (
-          <styled.span key={line} textStyle="label.01">
+        {message.split(/\r?\n/).map((line, index) => (


This was throwing a duplicate keys error

pete-watters · 2024-04-23T10:47:58Z

src/app/features/container/container.tsx

-        isShowing={isShowingSwitchAccount}
-        onClose={() => setIsShowingSwitchAccount(false)}
-      />
+      {isShowingSwitchAccount && (


There was a mistake here in that <SwitchAccountDialog shouldn't be in the DOM unless showing

nice catch! I believe problem with requests not being cancelled on close in SwitchAccountDialog is here

pete-watters · 2024-04-23T10:48:46Z

src/app/common/hooks/use-media-query.ts

@@ -0,0 +1,23 @@
+import { useEffect, useState } from 'react';


Re-adding this which I naively removed as I thought we didn't need it, however it's important as it alters the breakpoint on re-size

alter-eggo

great work 💪

src/app/common/hooks/use-media-query.ts

alter-eggo · 2024-04-24T09:41:25Z

src/app/features/container/container.tsx

-        isShowing={isShowingSwitchAccount}
-        onClose={() => setIsShowingSwitchAccount(false)}
-      />
+      {isShowingSwitchAccount && (


nice catch! I believe problem with requests not being cancelled on close in SwitchAccountDialog is here

alter-eggo · 2024-04-24T09:46:50Z

src/app/pages/choose-account/choose-account.tsx

@@ -34,12 +37,16 @@ export const ChooseAccount = memo(() => {
          {url && <RequesterFlag requester={url.toString()} />}
          <LogomarkIcon width="248px" height="58px" />
          <Stack gap="space.04">
-            <styled.h1 textStyle="heading.05">Choose an account to connect</styled.h1>
+            <styled.h1 textStyle="heading.05">


I believe we prob need some more explanation of what's going on for users?
something like 'you are connected with ledger in bitcoin mode only, please connect stacks account as well to use this feature' or smth like that

pete-watters force-pushed the fix/5242/virtuoso-dynamic-height branch from 94d01c6 to ab6dd7b Compare April 22, 2024 10:42

pete-watters commented Apr 23, 2024

View reviewed changes

pete-watters force-pushed the fix/5242/virtuoso-dynamic-height branch 2 times, most recently from a8698eb to 6e6765d Compare April 23, 2024 10:37

pete-watters commented Apr 23, 2024

View reviewed changes

pete-watters marked this pull request as ready for review April 23, 2024 10:50

pete-watters requested review from kyranjamie and alter-eggo April 23, 2024 10:50

pete-watters linked an issue Apr 23, 2024 that may be closed by this pull request

Can't scroll to last couple of accounts on the account selection screen #5242

Closed

fix: allow virtuoso to resize dynamically, ref #5242

6f242c3

pete-watters force-pushed the fix/5242/virtuoso-dynamic-height branch from 6e6765d to 6f242c3 Compare April 23, 2024 13:33

pete-watters requested a review from fbwoolf April 23, 2024 14:27

alter-eggo approved these changes Apr 24, 2024

View reviewed changes

pete-watters added this pull request to the merge queue Apr 24, 2024

Merged via the queue into dev with commit 5d618aa Apr 24, 2024
28 checks passed

pete-watters deleted the fix/5242/virtuoso-dynamic-height branch April 24, 2024 13:36

pete-watters linked an issue Apr 25, 2024 that may be closed by this pull request

Unable to scroll and select last accounts #5310

Closed

pete-watters mentioned this pull request Jun 28, 2024

Prevent flicker upon scroll to bottom of accounts list #4852

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow virtuoso to resize dynamically, ref #5242 #5289

fix: allow virtuoso to resize dynamically, ref #5242 #5289

pete-watters commented Apr 22, 2024 •

edited by leather-bot

Loading

pete-watters Apr 23, 2024

alter-eggo Apr 24, 2024

pete-watters Apr 24, 2024 •

edited

Loading

markmhendrickson Jul 24, 2024

pete-watters Jul 24, 2024

pete-watters Apr 23, 2024

pete-watters Apr 23, 2024

pete-watters Apr 23, 2024

pete-watters Apr 23, 2024

pete-watters Apr 23, 2024

pete-watters Apr 23, 2024

pete-watters Apr 23, 2024

pete-watters Apr 23, 2024

alter-eggo Apr 24, 2024

pete-watters Apr 23, 2024

alter-eggo left a comment

alter-eggo Apr 24, 2024

alter-eggo Apr 24, 2024

		const { decodedAuthRequest } = useOnboardingState();

		const { isShowingSwitchAccount, setIsShowingSwitchAccount } =

		@@ -0,0 +1,23 @@
		import { useEffect, useState } from 'react';

fix: allow virtuoso to resize dynamically, ref #5242 #5289

fix: allow virtuoso to resize dynamically, ref #5242 #5289

Conversation

pete-watters commented Apr 22, 2024 • edited by leather-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pete-watters Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alter-eggo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pete-watters commented Apr 22, 2024 •

edited by leather-bot

Loading

pete-watters Apr 24, 2024 •

edited

Loading