-
Notifications
You must be signed in to change notification settings - Fork 65
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
[VO-740] feat(nextcloud): Show trash #3187
Conversation
BundleMonFiles updated (3)
Unchanged files (15)
Total files change +249B 0% Groups updated (3)
Unchanged groups (4)
Final result: ✅ View report in BundleMon website ➡️ |
866c43d
to
0b45b89
Compare
0b45b89
to
e0abef6
Compare
This is preparatory work to share code between IOCozyFile routes and the Nextcloud routes. This also makes it easier to go backwards on Android.
e0abef6
to
824b070
Compare
try { | ||
setBusy(true) | ||
await onConfirm() | ||
} finally { |
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.
we don't handle the error scenario with some UI feedback here?
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.
Good point, I've added alerts in case of success or error
const handleConfirm = async (): Promise<void> => { | ||
if (!fileResult.data) return | ||
for (const file of fileResult.data) { | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call |
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.
I've seen multiple of these maybe it's easier to type cozy-client in d.ts for this method
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.
I don't really want to modify CozyClient
typing directly in Drive. We'd have to see how we could integrate this directly into the package, but we'd probably have to consider typing cozy-stack-client at the same time as .collection
is direct access to their classes.
@@ -118,7 +117,7 @@ const File = ({ | |||
// because they are magic folder created by the stack | |||
const canInteractWithFile = | |||
attributes._id !== 'io.cozy.files.shared-drives-dir' && | |||
attributes._id !== TRASH_DIR_ID | |||
!attributes._id.endsWith('.trash-dir') |
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.
why use a literal string instead of a constant here?
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 allows me to manage all the trash from both doctype io.cozy.files
and io.cozy.remote.nextcloud.files
in one line. You would have prefer something like that :
const canInteractWithFile =
attributes._id !== SHARED_DRIVES_DIR_ID &&
attributes._id !== TRASH_DIR_ID &&
attributes._id !== NEXTCLOUD_TRASH_DIR_ID
Maybe this constant should be provided by cozy-client or cozy-stack-client as they are already set as type here.
Note: I tend to put the doctype directly because I find them quicker to search globally in a project.
@@ -32,12 +36,21 @@ const NextcloudTrashFolderBody: FC<NextcloudTrashFolderBodyProps> = ({ | |||
window.open(file.links.self, '_blank') | |||
} | |||
|
|||
const fileActions = makeActions([], { | |||
const handleRefresh = async (files: NextcloudFile[]): Promise<void> => { |
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.
what is supposed to happen in case of throw during this refresh?
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.
I move this part inside the action directly so its handle by try/catch for the restore. I took advantage of this to limit the number of query resets by duplicating the parents' paths.
However, your comment made me realise that the resetQuery
function returns an error when the query didn't exist. I'm in the process of correcting it in cozy-client but I don't think it will be a problem for this PR.
state.fileIds?.includes(_id) | ||
) | ||
|
||
const handleConfirm = async (): Promise<void> => { |
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.
same here, what happens in case we can't handleConfirm? on ui side particularly (maybe it's irrelevant, not sure)
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.
The UI side is handle inside the DestroyConfirm
because the message are the same between io.cozy.files
and io.cozy.remote.nextcloud.files
This is preparatory work to share code between IOCozyFile routes and the Nextcloud routes. This also makes it easier to go backwards on Android.
824b070
to
10f846a
Compare
Related PRs :