fix: payment builder table loading state

[joanna-pagepro]

Description

Payment builder table loading state

Testing

• Step 1. Create a Payment builder action.
• Step 2. Add a few rows to the payment.
• Step 3. Create the payment, keeping an eye on the payment builder table.

Diffs

New stuff ✨

Changes 🏗

• loading state passed to RecipientField and UserPopover components

Deletions ⚰️

TODO

Resolves #2414

[marcocolony]

Hey @joanna-pagepro, thanks for looking into this!

The default address no longer shows up, but the loading state looks to be a bit inconsistent from what I have experienced.

Here's the expected behaviour for the loading state:

• You should see the skeleton loader with 3 rows until all the table data has loaded.

Actual current behaviour:

• Data seems to be displayed as it is loading
• Skeleton loading appears only for the exact data that has yet to be loaded
• If there's a row with data yet to load, all the other rows with complete data will still be visible

[davecreaser]

Nice one! Code-wide all good apart from one minor comment.

The skeleton loading state also looks good to me, but we might need to check with @JoinColony/product about Marco's comments on the exact requirements here.

[davecreaser]

A bit of a nitpick, but could we change the namings of these two variables to be more specific? Imo, loading and isLoading work fine as variable names if there's only one present, with them both it's a little confusing.

Perhaps something like loadingRecipientField and loadingMemberContext, just as an example. What do you think?

[jakubcolony]

Code-wise looks good (other than what Dave mentioned).

I'm also seeing the data appear incrementally and skeleton only showing on the user popover which is expected as the on-chain events are processed sequentially.

To show skeleton loader until all data is loaded you could store the expected count of payouts when expenditure is created and display the loader until the expenditure object from API has matching count of slots. You would need to check all of those slots have recipients set, and payouts with expected amount and claim delay. Feel free to ping me if you require more details.

[joanna-pagepro]

@jakubcolony The problem was that data related to the recipient was fetched inside the UserInfoPopover component, separately from the expenditure data. I used member context to get rid of this effect. Let me know if my solution is ok, or if I should revert the changes. I couldn't see any other issues, so if you see anything else, please also let me know.

[bassgeta]

Overall the changes are quite nice, we are slowly consolidating these popover components!
I dropped one comment, I think that we can stop passing isLoading to UserPopover and UserInfoPopover, since you made it that UserInfoPopover fetches data both from the member context and graphql

[bassgeta]

I actually think the solution you've implemented in UserInfoPopover fetches data from the same source, so we don't need to pass isLoading to UserPopover.
Maybe we could try removing that property. We can also try and see what happens if we revert the loading flag changes here (maybe it won't work though)

[joanna-pagepro]

@bassgeta it's used to pass the loading state of the data from the table, so I think this prop is also needed so that skeleton loader will be shown for all of the table items simultaneously. I also want to show the loading state on other items from the table when user data is being fetched.

[jakubcolony]

@jakubcolony The problem was that data related to the recipient was fetched inside the UserInfoPopover component, separately from the expenditure data. I used member context to get rid of this effect. Let me know if my solution is ok, or if I should revert the changes. I couldn't see any other issues, so if you see anything else, please also let me know.

I think that's fine, but please take a look at Kerry's comment as you might not need to pass the additional isLoading prop.

Nice work on this 👍

Screen.Recording.2024-06-10.at.19.50.28.mov

[joanna-pagepro]

@davecreaser @bassgeta can you review this one again?

[davecreaser]

All good to me except one small thing!

[davecreaser]

On line 65 (inside the useMemo) you still have isLoading={isLoading} which means there's now a missing dependency in the dependency array of this useMemo call.

You'll need to either keep isLoading as a dependency or change that line to isLoading={isDataLoading} to fix it. I assume it's just a missed thing and the second option is what you intended!

[melyndav]

This looks great @joanna-pagepro, thanks for your work on making this fix. 💯