feat: balance skeleton loader #5126

alter-eggo · 2024-03-26T13:34:18Z

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

This pr adds skeleton loader for balances on home page and switch account modal
https://github.com/leather-wallet/extension/assets/46521087/492e7c7f-49b4-446d-b99e-f25c1d080bda

src/app/components/account-total-balance.tsx

src/app/components/crypto-assets/crypto-currency-asset/crypto-currency-asset-item.layout.tsx

src/app/components/balance/balance-shimmer.tsx

pete-watters

I added some suggestions but overall good work and I like the shimmer 👍

kyranjamie · 2024-03-29T11:28:09Z

src/app/components/balance/balance-shimmer.tsx

+import { css } from 'leather-styles/css';
+import { Box } from 'leather-styles/jsx';
+
+export function BalanceShimmer({ ...rest }) {


Needs types

Also, this is the skeleton loader? Can we call it skeleton instead? Shimmer makes me think it's just the text effect

Also why does this need to be specific to balances? What if we need a skeleton loader for other non-balance text?

Suggest we make a skeleton design system component, with stories, rather than having a specific balance one. This component could wrap that?

yes, will rename to Skeleton loader. I actually added already some stories.
how you see this skeleton design system component? what should it take as props except width/height?

what should it take as props except width/height?

Yeah exactly. Either width/height, or maybe some pre-defined values? Not sure. cc/ @fabric-8 @mica000 to add to design system with variants needed

src/app/components/crypto-assets/crypto-currency-asset/crypto-currency-asset-item.layout.tsx

kyranjamie

Think it's important to not make this component specific to balance. Thoughts?

kyranjamie · 2024-03-29T11:37:29Z

src/app/components/balance/balance-shimmer.tsx

+import { css } from 'leather-styles/css';
+import { Box } from 'leather-styles/jsx';
+
+export function BalanceShimmer({ ...rest }) {


Also why does this need to be specific to balances? What if we need a skeleton loader for other non-balance text?

Suggest we make a skeleton design system component, with stories, rather than having a specific balance one. This component could wrap that?

src/app/components/account/account-name.tsx

markmhendrickson · 2024-04-02T08:43:42Z

Please link to a relevant issue and place that issue on the board 🙏

kyranjamie

SkeletonLoader component looks good, but think it's really important not to break the conceptual divide between a generic component, and an instance of its use.

src/app/components/balance/balance-shimmer.tsx

kyranjamie · 2024-04-04T08:27:19Z

src/app/ui/components/skeleton-loader/skeleton-loader.tsx

+  accountItemTotalBalance: {
+    height: '20px',
+  },
+  cryptoAssetItemTokenBalance: {
+    width: '126px',
+  },
+  cryptoAssetItemUsdBalance: {
+    width: '78px',
+  },
+  accountCardTotalBalance: {
+    width: '200px',
+    height: '38px',
+  },


This breaks separation of concerns. A generic component like this shouldn't know about all these specific use-cases within the wallet. Will list just grow every new use we have? IMO this is a really important rule not to break, and easy to avoid. Also, think how we want to refactor this to monorepo. Extension-specific UI concerns shouldn't be there either.

Why not make it configurable by passing width/height. Or, perhaps better, given that the skeleton loader is there to replace areas of text, why not pass the textStyle that it's masking and infer the height from that? And also pass a length for how many chars long it should be?

<SkeletonLoader textStyle="header.01" textLength={20} />

I made this to avoid duplication of width/height passing in components and stories.
maybe just put it to separate file? and yeah, just pass these styles in components to loader

I believe this textStyle approach is not extendable here as skeleton loader might be used not only for text

Yep you're right, let's skip the textstyle approach, as we don't want to assume it's only used for text.

To avoid the duplication, we should make wrapper components

AccountCardSkeletonLoader contains SkeletonLoader with some predefined widths and heights. That's a more flexible approach than baking in these concerns to the loader itself.

don't think we need any additional wrapper either. just pass width/height and that's it.
do we really need add to stories all skeleton usage variants?

src/shared/shimmer-styles.ts

kyranjamie

LGTM

314159265359879 · 2024-04-10T07:04:56Z

Was this already fixed? @alter-eggo you mentioned creating a new PR, I only see this one.

I see the loader for balances in this build but not for BNS names/account names on the switch select account screen.

Loading all the names on an account with hundreds of names looks good here!

alter-eggo · 2024-04-10T12:36:22Z

@314159265359879 seems there was problem with shimmer styles which fixed here #5205
that's why you haven't seen "loading" animation

markmhendrickson · 2024-04-10T16:42:29Z

The loading indicators themselves seem much too wide?

markmhendrickson · 2024-04-10T16:43:18Z

Also, is this branch not based on client-side throttling? I'm seeing lots of 429s

alter-eggo · 2024-04-10T18:09:04Z

@markmhendrickson it's quite an old pr, pls check #5206

alter-eggo changed the title ~~feat: balance shimmer loading~~ feat: balance skeleton loading Mar 26, 2024

alter-eggo changed the title ~~feat: balance skeleton loading~~ feat: balance skeleton loader Mar 26, 2024

alter-eggo force-pushed the feat/balance-loader branch 3 times, most recently from e438479 to 0860254 Compare March 28, 2024 14:41

alter-eggo marked this pull request as ready for review March 28, 2024 15:02

alter-eggo force-pushed the feat/balance-loader branch 2 times, most recently from 53a8b08 to 09f6c8d Compare March 28, 2024 17:56

alter-eggo requested review from kyranjamie, fbwoolf, pete-watters and fabric-8 March 28, 2024 17:59

pete-watters reviewed Mar 29, 2024

View reviewed changes

src/app/components/account-total-balance.tsx Outdated Show resolved Hide resolved

pete-watters reviewed Mar 29, 2024

View reviewed changes

src/app/components/account-total-balance.tsx Outdated Show resolved Hide resolved

pete-watters reviewed Mar 29, 2024

View reviewed changes

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

pete-watters reviewed Mar 29, 2024

View reviewed changes

src/app/components/balance/balance-shimmer.tsx Outdated Show resolved Hide resolved

pete-watters approved these changes Mar 29, 2024

View reviewed changes

kyranjamie reviewed Mar 29, 2024

View reviewed changes

kyranjamie requested changes Mar 29, 2024

View reviewed changes

fbwoolf reviewed Mar 29, 2024

View reviewed changes

src/app/components/account/account-name.tsx Outdated Show resolved Hide resolved

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

Add skeleton loaders for user balances #5119

Closed

alter-eggo force-pushed the feat/balance-loader branch from 09f6c8d to 1e6ce1a Compare April 4, 2024 08:08

alter-eggo requested review from kyranjamie and pete-watters April 4, 2024 08:08

alter-eggo force-pushed the feat/balance-loader branch from e0830c2 to 0f620f0 Compare April 4, 2024 08:14

kyranjamie reviewed Apr 4, 2024

View reviewed changes

feat: add balances shimmer loader, closes #5119

d0fd13a

alter-eggo force-pushed the feat/balance-loader branch from 0f620f0 to d0fd13a Compare April 4, 2024 12:00

alter-eggo requested a review from kyranjamie April 4, 2024 12:03

kyranjamie approved these changes Apr 4, 2024

View reviewed changes

alter-eggo added this pull request to the merge queue Apr 4, 2024

Merged via the queue into dev with commit 5c1c284 Apr 4, 2024
28 checks passed

alter-eggo deleted the feat/balance-loader branch April 4, 2024 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: balance skeleton loader #5126

feat: balance skeleton loader #5126

alter-eggo commented Mar 26, 2024 •

edited by leather-bot

Loading

pete-watters left a comment

kyranjamie Mar 29, 2024

kyranjamie Mar 29, 2024

kyranjamie Mar 29, 2024

alter-eggo Mar 29, 2024 •

edited

Loading

kyranjamie Mar 29, 2024

kyranjamie left a comment

kyranjamie Mar 29, 2024

markmhendrickson commented Apr 2, 2024

kyranjamie left a comment

kyranjamie Apr 4, 2024 •

edited

Loading

alter-eggo Apr 4, 2024

kyranjamie Apr 4, 2024

alter-eggo Apr 4, 2024 •

edited

Loading

kyranjamie left a comment

314159265359879 commented Apr 10, 2024

alter-eggo commented Apr 10, 2024

markmhendrickson commented Apr 10, 2024

markmhendrickson commented Apr 10, 2024

alter-eggo commented Apr 10, 2024

feat: balance skeleton loader #5126

feat: balance skeleton loader #5126

Conversation

alter-eggo commented Mar 26, 2024 • edited by leather-bot Loading

pete-watters left a comment

Choose a reason for hiding this comment

kyranjamie Mar 29, 2024

Choose a reason for hiding this comment

kyranjamie Mar 29, 2024

Choose a reason for hiding this comment

kyranjamie Mar 29, 2024

Choose a reason for hiding this comment

alter-eggo Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

kyranjamie Mar 29, 2024

Choose a reason for hiding this comment

kyranjamie left a comment

Choose a reason for hiding this comment

kyranjamie Mar 29, 2024

Choose a reason for hiding this comment

markmhendrickson commented Apr 2, 2024

kyranjamie left a comment

Choose a reason for hiding this comment

kyranjamie Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

alter-eggo Apr 4, 2024

Choose a reason for hiding this comment

kyranjamie Apr 4, 2024

Choose a reason for hiding this comment

alter-eggo Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

kyranjamie left a comment

Choose a reason for hiding this comment

314159265359879 commented Apr 10, 2024

alter-eggo commented Apr 10, 2024

markmhendrickson commented Apr 10, 2024

markmhendrickson commented Apr 10, 2024

alter-eggo commented Apr 10, 2024

alter-eggo commented Mar 26, 2024 •

edited by leather-bot

Loading

alter-eggo Mar 29, 2024 •

edited

Loading

kyranjamie Apr 4, 2024 •

edited

Loading

alter-eggo Apr 4, 2024 •

edited

Loading