feat: add manage tokens, closes #5643 #5896

alter-eggo · 2024-10-07T08:29:22Z

Try out Leather build f7e50fe — Extension build, Test report, Storybook, Chromatic

This pr adds:

manage tokens feature
mocks all token types (sip10, runes, brc20, stx20, src20) and tests to enable/disable all of them

@cbadawi thanks a lot for working on this feature!

brandonmarshall-tm · 2024-10-07T20:39:29Z

I downloaded the preview and am noticing all my Runes are hidden by default, even though we support USD prices on Runes. Ideally we would filter out low-quality Runes but keep ones that have higher value in wallets such as $DOG (DOG GO TO MOON)

brandonmarshall-tm · 2024-10-07T20:41:47Z

I am also seeing a bug where SRC20 is showing up even when unchecked

brandonmarshall-tm · 2024-10-08T00:20:15Z

I just noticed that even when enabling Runes, they do not show in the asset list. So Runes are always hidden and SRC20 are always shown 😉

alter-eggo · 2024-10-08T08:57:05Z

@brandonmarshall-tm fixed

brandonmarshall-tm · 2024-10-08T21:21:38Z

Confirming I am able to hide SRC20 and unhide Runes. All Runes are still hidden by default, though, which I don't think is our intention

brandonmarshall-tm · 2024-10-09T21:22:22Z

Looks good to me!

src/app/components/crypto-asset-item/crypto-asset-item-toggle.layout.tsx

src/app/features/asset-list/stacks/stx-crypo-asset-item/stx-crypto-asset-item.tsx

src/app/features/asset-list/asset-list.tsx

src/app/common/hooks/use-manage-tokens.ts

kyranjamie · 2024-10-10T12:46:09Z

src/app/features/asset-list/bitcoin/src20-token-asset-list/src20-token-asset-list.tsx

+    if (assetRightElementVariant === 'toggle') {
+      return (
+        <CryptoAssetItemToggleLayout
+          key={key}
+          captionLeft={captionLeft}
+          icon={icon}
+          titleLeft={titleLeft}
+          assetId={token.info.symbol}
+          isCheckedByDefault={isTokenEnabled({ tokenId: token.info.symbol, preEnabledTokensIds })}
+        />
+      );
+    }
+    return (
+      <CryptoAssetItemLayout
+        availableBalance={token.balance.availableBalance}
+        captionLeft={captionLeft}
+        key={key}
+        icon={<Src20AvatarIcon />}
+        titleLeft={titleLeft}
+        isPrivate={isPrivate}
+        dataTestId={token.info.symbol}
+      />
+    );
+  });


This approach seems a coupled to me. Rather than "baking in" the toggleable functionality to every single list, shouldn't we compose together a new view that has the toggle functionnality?

refactored this, so it's in sep component

kyranjamie · 2024-10-10T12:47:01Z

src/app/store/manage-tokens/manage-tokens.slice.ts

+  name: 'manageTokens',
+  initialState: manageTokensAdapter.getInitialState(),
+  reducers: {
+    setToken(state, action: PayloadAction<{ id: string; enabled: boolean; accountIndex: number }>) {


Can we model over the action/event taking place? https://redux.js.org/style-guide/#model-actions-as-events-not-setters

Agree here, trying to understand some of the changes, is it nec to have manage in the naming? Isn't a slice of state always managing state, would just tokensSlice be sufficient here to describe it? I think there might be a way to make what is happening here more clear, like what is setToken doing?

manageToken more refer to a feature itself, not like managing tokens state slice

314159265359879 · 2024-10-15T08:59:17Z

@alter-eggo I think I was looking at a different version when I tested this in the weekend. This is looking good now.

I would prefer the settings to be the same for all accounts instead of different per account. But as you said that can be an enhancement for later so that this can go live as it is now.

Should there be an option to easily hide/unhide all (except gas tokens) too?

The color difference between on and off is very small, that makes the color difference less helpful to see the difference.

alter-eggo · 2024-10-15T10:03:20Z

@314159265359879 we can add "all" switcher as further enhancement as well 👍 let's create sep issue for these ideas?

The color difference between on and off is very small, that makes the color difference less helpful to see the difference.

hm, as for me it looks ok, wdyt @fabric-8?
bg is ink.text-subdued in unchecked mode and ink.text-primary in checked mode

kyranjamie · 2024-10-15T10:32:18Z

src/app/store/manage-tokens/manage-tokens.slice.ts

+    updateTokenEnableState(
+      state,
+      action: PayloadAction<{ id: string; enabled: boolean; accountIndex: number }>
+    ) {
+      manageTokensAdapter.setOne(state, {
+        id: action.payload.id,
+        enabled: action.payload.enabled,
+        accountIndex: action.payload.accountIndex,
+      });
+    },


Suggested change

updateTokenEnableState(

state,

action: PayloadAction<{ id: string; enabled: boolean; accountIndex: number }>

) {

manageTokensAdapter.setOne(state, {

id: action.payload.id,

enabled: action.payload.enabled,

accountIndex: action.payload.accountIndex,

});

},

updateTokenEnableState(

state,

action: PayloadAction<{ id: string; enabled: boolean; accountIndex: number }>

) {

manageTokensAdapter.setOne(state, {

id: action.payload.id,

enabled: action.payload.enabled,

accountIndex: action.payload.accountIndex,

});

},

This still kinda violates the "map to user actions, not state setters" practice. I'd go for something like userTogglesTokenVisibility etc

pete-watters

Great work @alter-eggo 👍

fabric-8 · 2024-10-15T11:26:38Z

Great stuff — Let's revisit the toggle component states separately, I think it's ok for now to move on

alter-eggo force-pushed the feat/tokens-manage branch 2 times, most recently from b75a018 to 2c46b31 Compare October 7, 2024 18:37

alter-eggo force-pushed the feat/tokens-manage branch from 2c46b31 to b861945 Compare October 8, 2024 06:07

alter-eggo force-pushed the feat/tokens-manage branch 3 times, most recently from 1430dd0 to 0861792 Compare October 9, 2024 20:05

alter-eggo marked this pull request as ready for review October 9, 2024 20:11

alter-eggo requested review from fbwoolf, kyranjamie and edgarkhanzadian and removed request for kyranjamie, fbwoolf and edgarkhanzadian October 10, 2024 09:21

alter-eggo force-pushed the feat/tokens-manage branch from 0861792 to 33d8d66 Compare October 10, 2024 09:28

alter-eggo requested review from fbwoolf, kyranjamie and edgarkhanzadian October 10, 2024 09:28

alter-eggo mentioned this pull request Oct 10, 2024

feat: manage tokens #5568

Closed

alter-eggo requested a review from pete-watters October 10, 2024 10:01

pete-watters reviewed Oct 10, 2024

View reviewed changes

src/app/components/crypto-asset-item/crypto-asset-item-toggle.layout.tsx Outdated Show resolved Hide resolved

pete-watters reviewed Oct 10, 2024

View reviewed changes

src/app/features/asset-list/stacks/stx-crypo-asset-item/stx-crypto-asset-item.tsx Outdated Show resolved Hide resolved

pete-watters reviewed Oct 10, 2024

View reviewed changes

src/app/features/asset-list/asset-list.tsx Outdated Show resolved Hide resolved

kyranjamie reviewed Oct 10, 2024

View reviewed changes

alter-eggo force-pushed the feat/tokens-manage branch from 33d8d66 to fe2538d Compare October 11, 2024 15:36

alter-eggo force-pushed the feat/tokens-manage branch 4 times, most recently from bcbd817 to f46def1 Compare October 15, 2024 07:26

alter-eggo requested review from kyranjamie and pete-watters October 15, 2024 07:27

kyranjamie approved these changes Oct 15, 2024

View reviewed changes

feat: add manage tokens, closes #5643

f7e50fe

alter-eggo force-pushed the feat/tokens-manage branch from f46def1 to f7e50fe Compare October 15, 2024 10:44

This was referenced Oct 15, 2024

Token management: make show/hide status per token a state that is used for all accounts #5903

Open

Token management: Add button to show/hide all tokens with a single button #5904

Open

pete-watters approved these changes Oct 15, 2024

View reviewed changes

alter-eggo added this pull request to the merge queue Oct 15, 2024

Merged via the queue into dev with commit 7d3ac6f Oct 15, 2024
30 checks passed

alter-eggo deleted the feat/tokens-manage branch October 15, 2024 11:26

alter-eggo linked an issue Oct 17, 2024 that may be closed by this pull request

Add option to manage tokens shown or hidden by default #5643

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add manage tokens, closes #5643 #5896

feat: add manage tokens, closes #5643 #5896

alter-eggo commented Oct 7, 2024 •

edited by leather-bot

Loading

brandonmarshall-tm commented Oct 7, 2024

brandonmarshall-tm commented Oct 7, 2024

brandonmarshall-tm commented Oct 8, 2024

alter-eggo commented Oct 8, 2024

brandonmarshall-tm commented Oct 8, 2024

brandonmarshall-tm commented Oct 9, 2024

kyranjamie Oct 10, 2024

alter-eggo Oct 13, 2024

kyranjamie Oct 10, 2024

fbwoolf Oct 10, 2024

alter-eggo Oct 11, 2024 •

edited

Loading

314159265359879 commented Oct 15, 2024

alter-eggo commented Oct 15, 2024 •

edited

Loading

kyranjamie Oct 15, 2024

pete-watters left a comment

fabric-8 commented Oct 15, 2024

feat: add manage tokens, closes #5643 #5896

feat: add manage tokens, closes #5643 #5896

Conversation

alter-eggo commented Oct 7, 2024 • edited by leather-bot Loading

brandonmarshall-tm commented Oct 7, 2024

brandonmarshall-tm commented Oct 7, 2024

brandonmarshall-tm commented Oct 8, 2024

alter-eggo commented Oct 8, 2024

brandonmarshall-tm commented Oct 8, 2024

brandonmarshall-tm commented Oct 9, 2024

kyranjamie Oct 10, 2024

Choose a reason for hiding this comment

alter-eggo Oct 13, 2024

Choose a reason for hiding this comment

kyranjamie Oct 10, 2024

Choose a reason for hiding this comment

fbwoolf Oct 10, 2024

Choose a reason for hiding this comment

alter-eggo Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

314159265359879 commented Oct 15, 2024

alter-eggo commented Oct 15, 2024 • edited Loading

kyranjamie Oct 15, 2024

Choose a reason for hiding this comment

pete-watters left a comment

Choose a reason for hiding this comment

fabric-8 commented Oct 15, 2024

alter-eggo commented Oct 7, 2024 •

edited by leather-bot

Loading

alter-eggo Oct 11, 2024 •

edited

Loading

alter-eggo commented Oct 15, 2024 •

edited

Loading