Dashboard: ActivitiesList component
#400
Conversation
Added dismiss animation, updated `LoadingSpinnerSuccessIcon` component
kpyszkowski
requested review from
r-czajkowski and
kkosiorowska
and removed request for
r-czajkowski
✅ Deploy Preview for acre-dapp-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
dapp/src/components/shared/StakingActivitiesList/StakingActivitiesList.tsx
Outdated
Show resolved
Hide resolved
dapp/src/components/shared/StakingActivitiesList/StakingActivitiesList.tsx
Outdated
Show resolved
Hide resolved
dapp/src/components/shared/StakingActivitiesList/StakingActivitiesListItem.tsx
Outdated
Show resolved
Hide resolved
dapp/src/components/shared/StakingActivitiesList/StakingActivitiesList.tsx
Outdated
Show resolved
Hide resolved
| const handleItemDismiss = (id: string) => { | ||
| setDismissedItemIds((prev) => [...prev, id]) | ||
| } |
There was a problem hiding this comment.
We pass this function to the child component. So let's useCallback here.
There was a problem hiding this comment.
Generally speaking the rule is to optimize code only when you have performance issues.
Wrapping every callback with useCallback is an anti pattern which can have the opposite effect to that intended. Memoization costs, you always need to think twice before you decide to memoize a value. Very often it's not prevention but over engineering with possible harmful results.
There was a problem hiding this comment.
If handleItemDismiss would operate on hardly reactive data it would be reasonable to memoize this callback. But in this particular case the only value we might want to memoize is state setter which is just a function - static (not reactive) function.
There was a problem hiding this comment.
It's true that we shouldn't wrap everything in useCallback or useMemo. In this case, we have a list of items and we pass this function to the children. Therefore, I would be inclined to do it. Note that we will not use the setState function here in the final app version.
What is interesting is that information that setState function identity is stable has been omitted from the new documentation. That's why I have mixed feelings sometimes.
| import { CurrencyBalance } from "../CurrencyBalance" | ||
| import Spinner from "../Spinner" | ||
|
|
||
| export type StakingActivitiesListItemType = { |
There was a problem hiding this comment.
We already have a defined type for the activity. I will be updating this type soon so I will add missing fields such as timestamp or id.
acre/dapp/src/types/activity.ts
Lines 16 to 21 in b4d3951
There was a problem hiding this comment.
We can use tx hash as id. I did so.
There was a problem hiding this comment.
Currently, we can do it like that. We will probably have to use an id since we don't have a txHash yet when the deposit is initialized. But we can fix it later.
`StakingActivitiesList` to `ActivitiesList`
Adjusted container items alignment
Rendered `ActivitiesList` in `DashboardCard`. Refactored `BlockExplorerLink` to make it compatible with `as` prop
kpyszkowski
changed the title
StakingActivitiesList componentActivitiesList component
dapp/src/components/shared/ActivitiesList/ActivitiesListItem.tsx
Outdated
Show resolved
Hide resolved
Closes: #505 This PR resolves an issue with inconsistency in `Alert` components. ### Changes: - Now styles are defined as Chakra theme only. - Components from `shared/alerts/...` have been removed in favor of `shared/Alert` component. - `ErrorAlert` component has been removed and replaced by `<Alert status="error" />` component. - `CardAlert` component has been removed and replaced by `<Alert variant="elevated" />` component. - The `process` variant has been ported from #400 **Note:** Changes are not fully accurate with designs (especially in color levels) because of inconsistency. It can be adjusted in the future as the design system will be introduced.
Closes: #363
Goal:
To implement
ActivitiesListcomponentFeatures:
statuspropertyDesigns:
Testing:
dashboard-page.patch