[REFACTOR] History data fetching #1698
Conversation
| await fetchData(); | ||
| }; | ||
| getData(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
If we use fetchData as a dep of the useEffect it triggers another load of the data. React officially recommends using an empty array to run effects only on mount.
I think we should remove this lint rule but if we really don't want to we can also use the loading state to step around the extra call, but we won't be able to avoid the extra render.
There was a problem hiding this comment.
I'm fine using the empty array as useEffect dependency and disabling the lint rule on a case-by-case manner, I think it makes sense for cases where we need to run the code only once on mount 👍
I'm on the fence about removing the lint rule from the project, it could be useful in other cases where it's important to add more deps
There was a problem hiding this comment.
yeah good point, it has helped me spot missing deps before so maybe it's ok to just step around it for these cases?
There was a problem hiding this comment.
Agreed - let's keep the lint rule and just disable when we explicitly just want to run on mount
| const showLoader = | ||
| getHistoryState.state === RequestState.IDLE || | ||
| getHistoryState.state === RequestState.LOADING; | ||
| const hasEmptyHistory = !showLoader || !getHistoryState.data; |
There was a problem hiding this comment.
should we be checking for data.length here?
do we really need to check the showLoader value here?
e.g.:
const hasEmptyHistory = !getHistoryState?.data?.length;
There was a problem hiding this comment.
we can't rely on the history length alone because then we would show this message during loading or idle states. I believe the correct logic is "show this when data is not loading and there is no history".
There was a problem hiding this comment.
got it, so maybe something like this?
const hasEmptyHistory = !showLoader && !getHistoryState?.data?.length;
There was a problem hiding this comment.
yeah good call to cover the empty array case as well, updating to this!
There was a problem hiding this comment.
actually since you suggested we pull out the loading render branch, it did make it easier to use your original suggestions. Updated in c1612f5
What do these cases look like now? |
| return <TransactionDetail {...detailViewProps} />; | ||
| } | ||
|
|
||
| return showLoader ? ( |
There was a problem hiding this comment.
I think it's totally up to you but just leaving a little suggestion here. I personally think avoiding the ternary operator for big chunks of code reads a little bit better. E.g.:
if (isDetailViewShowing) {
return <TransactionDetail {...detailViewProps} />;
}
if (showLoader) {
return <Loading />;
}
return <View.Content>
...
There was a problem hiding this comment.
oh yeah good call, I pulled out isDetailViewShowing but not sure why I didn't pull out the loading state. I'll make that change. 👍
There was a problem hiding this comment.
updated in 8812240
| } | ||
| }; | ||
|
|
||
| function useGetHistory(publicKey: string, networkDetails: NetworkDetails) { |
There was a problem hiding this comment.
thanks for implementing this!
There was a problem hiding this comment.
It's a little different than the example we discussed in the issue but this seemed easy to work with, open to any feedback on this pattern!
The empty case just prints "No transactions to show" and an error to fetch history results in an empty history pane. I don't think we've ever had UI state for errors on this screen. |
| @@ -0,0 +1,85 @@ | |||
| import { useReducer } from "react"; | |||
There was a problem hiding this comment.
NIT: can you call this file useGetHistory.tsx to conform to the naming convention in the rest of the repo?
There was a problem hiding this comment.
whoops old habit, renamed in be39fcd
| isHideDustEnabled, | ||
| ]); | ||
|
|
||
| const showLoader = |
There was a problem hiding this comment.
NIT: let's call this isLoaderShowing to follow code convention
There was a problem hiding this comment.
yup, renamed in 68003d3
Closes #1672
What
Adds
use-get-historyhook to fetch history items.Uses it in AccountHistory to replace in view data fetching.
Why
We've talked about moving to a hook based data fetching pattern for views that need fresh data on every render. Also, account history suffers from a glitchy loading state due to it's current data fetching/loading pattern.
Todo
We should follow up with design tasks for error and empty states for this screen.
Screen.Recording.2024-11-25.at.10.59.25.AM.mov