Pull: Move progress functions to the existing progress custom hook - #1380
Conversation
bcotrim
left a comment
bcotrim
left a comment
There was a problem hiding this comment.
Works as described
LGTM 👍
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
I left a couple of comments on the code. I know a lot of that stuff was the same prior to this PR, but this seems like a good opportunity to address it.
If you're not already familiar with the ProgressBarWithAutoIncrement component, I'd also like to mention that. We use it for preview sites and it provides some nice interactivity. Granted, sync operations typically take longer, but you could give it a small increment prop to account for that.
| progress: | ||
| IMPORTING_INITIAL_VALUE + IMPORTING_TO_FINISHED_STEP * ( importState.progress / 100 ), |
There was a problem hiding this comment.
What do we accomplish with this calculation? IMPORTING_INITIAL_VALUE is always 80 and IMPORTING_TO_FINISHED_STEP is always 20, so it looks like we're saying 100 * (80 / 100). Why not just return importState.progress..?
There was a problem hiding this comment.
Not really, 80 + 20 * progress / 100 != progress 😛 . This tries to move from 80% to 100% progressively determined by importState.progress. I think I can add a comment there to clarify, as it can be difficult to read.
There was a problem hiding this comment.
Ofc, my bad 🤦♂️ Doesn't seem as bad now that I come back to it, though. Maybe assigning the IMPORTING_TO_FINISHED_STEP * ( importState.progress / 100 ) part to a separate variable first would be enough to make it easier to understand.
| newProgressInfo.progress = | ||
| IN_PROGRESS_INITIAL_VALUE + IN_PROGRESS_TO_DOWNLOADING_STEP * ( response.percent / 100 ); |
There was a problem hiding this comment.
Same reasoning, I think some comments will help here, I'm adding them
|
|
||
| const getBackupStatusWithProgress = useCallback( | ||
| ( | ||
| hasBackupCompleted: boolean, |
There was a problem hiding this comment.
Couldn't we intuit this value inside getBackupStatusWithProgress by checking if response.status === 'finished'?
There was a problem hiding this comment.
Good point, I originally passed it as a parameter because it was already obtained here, but in this case, it could be better to calculate it the function and avoid the extra parameter, do you agree?
| export type SyncBackupResponse = { | ||
| status: 'in-progress' | 'finished' | 'failed'; | ||
| download_url: string; | ||
| percent: number; | ||
| }; | ||
|
|
There was a problem hiding this comment.
This type is also defined in src/hooks/sync-sites/use-sync-pull.ts. Could we use a single definition, and, while we're at it, maybe convert it to a zod schema?
There was a problem hiding this comment.
Good catch, I meant to delete it from src/hooks/sync-sites/use-sync-pull.ts and declare it here instead to avoid a circular dependency. Alternatively, I can move it to an external types file and import it from there in both places (here and in use-sync-pull). Do you prefer one option over the other?
There was a problem hiding this comment.
For reference, I've deleted the redundant definition in this commit 2241fd6
…ibility Co-authored-by: Fredrik Rombach Ekelund <fredrik@f26d.dev>
Co-authored-by: Fredrik Rombach Ekelund <fredrik@f26d.dev>
|
I will merge this as I have addressed most of the comments. If there are any other suggestions, please let me know, and I will create a follow-up task for that |
3 participants
Related issues
Proposed Changes
getBackupProgressandgetStatusWithProgressto the existinguseSyncStatesProgressInfohook, which exposes utility functions to return Progress. This seems to be a better location for those functions, more consistent with existing code.Testing Instructions
npm startPre-merge Checklist