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

Wallet Switcher v2 #6318

Merged
merged 58 commits into from
Jan 8, 2025
Merged

Wallet Switcher v2 #6318

merged 58 commits into from
Jan 8, 2025

Conversation

maxbbb
Copy link
Contributor

@maxbbb maxbbb commented Dec 10, 2024

Fixes APP-2081

Spec: https://www.notion.so/135b001b85b4808cae11f62380e8fc5c

What changed (plus any additional context for devs)

  • Extensions and fixes to the drag and drop component (originally based on this: https://github.com/mgcrea/react-native-dnd).
    • Adds DraggableScrollView and the useDraggableScroll hook that handles auto scrolling when dragging items at the visible edge of the scroll.
    • Refactors offset calculation for DraggableGrid to fix bug related to offsets drifting when interacting too quickly with different draggable items. This fix needs to also be applied to the DraggableScrollView but it would currently break auto scrolling
    • Fixes gesture blocking issues
  • Main changes to previously existing files are to ChangeWalletSheet, WalletList, and AddressRow
  • Adds PinnedWalletsGrid for the grid of up to 6 pinned wallets.
  • Adds zustand store for managing pinned wallets pinnedWalletsStore
  • Extends DropdownMenu component to be usable for non checkmark type items
  • Adds FeatureHintTooltip for general feature hints, API loosely based on Radix Tooltip component.

Review Notes

  • Some of the renaming / file movement didn't get tracked properly. Most everything is new code, but the core functions in the ChangeWalletSheet related to removing, editing, etc. a wallet did not change.

Known Issues

  • When scrolled down at all, the header is not draggable to dismiss the sheet. This problem exists in the current version as well and other panel sheets in the app, don't know why it happens.
  • The dropdown menu steals the onLongPress callback and haptic somehow from the button press animation
  • I cannot reproduce consistently, but a couple times when removing a wallet, the others would get stuck in a state of "balance loading" for the balance label. This is likely not a newly introduced bug, but I have not tested for this in prod. (fixed)
  • Scroll indicator bottom inset not working
  • The easing gradient on the footer is a stand in for the variable blur on iOS, but is what it will be on Android. Currently the total balance text does not stand out enough on top of it, the design of this should be rethought a little.

Potential follow up improvements

  • Calculate exact height of sheet and animate height changes when pinned / unpinned sections change in length.
  • Add feature to drag and drop component that clones children and animates away removed children and animates remaining children to new resting offsets. Currently, when the number of children in a Draggable changes, all the offsets of the remaining children are reset with no animation.
  • Use useSyncSharedValue for editMode so AddressRow layout transitions can be smoothly animated
  • Custom gesture handler to animate height from initial smaller snap point to large snap point before the scrollview gesture can begin activation
  • Integrate variable blur package and replace the footer's EasingGradient with a BlurView on iOS.
  • The "Wallet Settings" option in the Dropdown menu currently takes you to the general wallets and backup setting page, as that page is not currently setup to handle deep linking to a specific wallet.
  • Copy toast when context menu copy address is pressed
  • The DndProvider currently does some custom handling of the pan gesture starting, which it should not. This is the source of some issues where pressing on the item gives haptic feedback indicating its ready to drag but it cannot be dragged because the gesture actually failed. (implemented)

Screen recordings / screenshots

RPReplay_Final1734630484.MP4

What to test

  • You should start off with an initial set of pinned wallets (if you have any that you have imported). Which ones get default pinned is based on rainbow originated transactions, which is currently returned as a placeholder number from the backend until it is implemented.
  • Switching to different wallets
  • Adding new wallet
  • Pinning & unpinning
  • Long pressing wallet items in non edit mode for new context menu
  • Dragging around different accounts then unpinning / pinning them

Copy link

linear bot commented Dec 10, 2024

@walmat
Copy link
Contributor

walmat commented Dec 20, 2024

Not sure if intentional, but I had 2 wallets that weren't read only, and 2 that were, and only 2 wallets got pinned.
Screenshot 2024-12-20 at 10 24 34 AM

@walmat
Copy link
Contributor

walmat commented Dec 20, 2024

Only blocker, total wallet balance at the bottom for me is $NaN. Happy to help investigate if needed!
Screenshot 2024-12-20 at 10 24 10 AM

@maxbbb
Copy link
Contributor Author

maxbbb commented Dec 20, 2024

  1. Agreed, tweaked it
  2. Yes that is intentional, was in the spec to not auto pin watched wallets
  3. Fixed the NaN thing, was a bug in the addDisplay utility function

@brunobar79
Copy link
Member

Launch in simulator or device for 35bd715

@brunobar79
Copy link
Member

Launch in simulator or device for e425d4b

@jinchung jinchung removed the release for release blockers and release candidate branches label Dec 20, 2024
@brunobar79
Copy link
Member

Launch in simulator or device for 7202111

@brunobar79
Copy link
Member

Launch in simulator or device for d5589a1

Copy link
Member

@derHowie derHowie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, i played with it a lot and it looks solid but I think I found one issue. When I unpinned all wallets, then repinned them, the draggable functionality seemed to break.

@brunobar79
Copy link
Member

Launch in simulator or device for 2edd9aa

@derHowie
Copy link
Member

derHowie commented Jan 8, 2025

hey, @maxbbb

I tested this again, but I think I'm seeing more issues dragging now than before. I logged the last commit. Let me know if you think this issue is on my end.

Pinning 3 wallets also seems to lock up dragging. https://www.loom.com/share/14ab2236f96942c48de0300d94f5f7e1

@maxbbb
Copy link
Contributor Author

maxbbb commented Jan 8, 2025

hey, @maxbbb

I tested this again, but I think I'm seeing more issues dragging now than before. I logged the last commit. Let me know if you think this issue is on my end.

Pinning 3 wallets also seems to lock up dragging. https://www.loom.com/share/14ab2236f96942c48de0300d94f5f7e1

I've been testing on real device so I will test on simulator to see if any issue is more obvious there.

However, I think what's happening in your video is expected behavior. There is a 150ms delay where you have to press on the item before the drag can become active. There has to be some delay here, otherwise you wouldn't be able to scroll the scrollview. This value could be adjusted to be slightly lower if it doesn't feel right, but I tried to match the perceived behavior of moving apps around your home screen on iOS. From the video it looks like you're clicking then immediately dragging before that 150ms window, but if that's not what's happening though let me know.

@brunobar79 brunobar79 added the release for release blockers and release candidate branches label Jan 8, 2025
@derHowie
Copy link
Member

derHowie commented Jan 8, 2025

@maxbbb I think you're correct. The behavior feels much better on simulator when I'm more patient. I tested on a physical device and the haptic feedback makes the draggable window feel very clear.

Copy link
Member

@derHowie derHowie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fabulous, ✅

@walmat walmat self-requested a review January 8, 2025 19:54
@maxbbb maxbbb merged commit 9e2a040 into develop Jan 8, 2025
9 checks passed
@maxbbb maxbbb deleted the @kane/APP-2081 branch January 8, 2025 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release for release blockers and release candidate branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants