-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better import in progress #2027
Conversation
Please take the time to write a PR description :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really nice 👏 , the real blocker for me is the lack of test for the JobsContextProvider though.
I have some remarks and nits.
useEffect(() => { | ||
startJobsRealtime() | ||
return () => stopJobsRealtime() | ||
}, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is useRealtime hook that could simplify the logic, it does the start/stop/subscribe/unsubscribe for you:
cozy-ui might not be the good place for it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think so too, it would be better in realtime lib
return ( | ||
<JobsContext.Provider | ||
value={{ | ||
jobsInProgress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: It could be nice to prepare a bit the context value to make its consumption easier for the components below. For example, indexing the accounts or the triggers, or the connectors.
const value = useMemo(() => ({
inProgress: jobsInProgress,
byAccountId: keyBy(jobsInProgress, x => x.account),
byKonnectorSlug: keyBy(jobsInProgress, x => x.konnector),,
byTriggerId: keyBy(jobsInProgress, x => x.trigger), // useMemo could be used
}, [jobsInProgress])
return <JobsContext.Provider
value={value}
...
/>
The value passed below is here memoized not to incure unnecessary renders (not sure if this is really necessary but it cannot hurt I guess) and indexed so that a component can access info easily.
336c477
to
ae5e5da
Compare
add tests for JobsContextcozy-banks/src/components/JobsContext.jsx Lines 23 to 28 in ae5e5da
This comment was generated by todo based on a
|
ac31d14
to
3e6c596
Compare
use 'useQuery' instead of 'client'cozy-banks/src/ducks/balance/ImportGroupPanel.jsx Lines 125 to 130 in 3e6c596
This comment was generated by todo based on a
|
3e6c596
to
02a303d
Compare
use 'useQuery' instead of 'client'cozy-banks/src/ducks/balance/ImportGroupPanel.jsx Lines 125 to 130 in 02a303d
This comment was generated by todo based on a
|
We want to view when an account import is in progress on the balance page and in the settings page. For that we activate the realtime on the jobs when we are on one of these 2 pages otherwise we stop it. We also want to display a notification to the user whether it went correctly or not. There are several tests which are not added yet
02a303d
to
e932082
Compare
use 'useQuery' instead of 'client'cozy-banks/src/ducks/balance/ImportGroupPanel.jsx Lines 125 to 130 in e932082
This comment was generated by todo based on a
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks !
We want to view when an account import is in
progress on the balance page and in the settings
page. For that we activate the realtime on the jobs
when we are on one of these 2 pages otherwise we
stop it. We also want to display a notification to
the user whether it went correctly or not.