-
Notifications
You must be signed in to change notification settings - Fork 16
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
Sharing file browser #1048
Sharing file browser #1048
Conversation
Your Render PR Server URL is https://files-ui-stage-pr-1048.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-c2jnu9b2v8panf9vk5qg. |
We said we'd put this on the side for the time being. Closing so as not to pollute our pulls list. |
Oops I meant the other one sorry. |
Your Render PR Server URL is https://files-ui-stage-pr-1048.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-c36bon85ou7u4hodod40. |
Your Render PR Server URL is https://storage-ui-stage-pr-1048.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-c36boo05ou7u4hododkg. |
This pull request introduces 3 alerts when merging 90586f1 into c5c925f - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging e5d9c61 into c5c925f - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging 6fad93e into c5c925f - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging 6dec91e into c5c925f - view on LGTM.com new alerts:
|
…/files-ui into feat/sharing-file-browser-1043
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 is working damn well! Went through the code quickly, left a couple comments there, and what I came across:
- Getting this in the console
Warning: Each child in a list should have a unique "key" prop.
Check the render method of `FileSystemTableItem`. See https://fb.me/react-warning-keys for more information.
in span (at FileSystemTableItem.tsx:252)
in FileSystemTableItem (at FileSystemItem.tsx:413)
in FileSystemItem (at SharesList.tsx:905)
- When moving a file, the tree is not the right one (it's the one of not shared folders). (edit, made an issue out of it Move files inside shared browser isn't showing the right tree #1204)
- As discussed deleting a file should delete it altogether. (made an issue out of it Shared files and folder should be deleted directly and not go in the bin #1203)
packages/files-ui/src/Components/Modules/FileBrowsers/Sharing/ShareFileBrowser.tsx
Outdated
Show resolved
Hide resolved
...es/files-ui/src/Components/Modules/FileBrowsers/views/FileSystemItem/FileSystemTableItem.tsx
Outdated
Show resolved
Hide resolved
packages/files-ui/src/Components/Modules/FileBrowsers/SharedFolderRowWrapper.tsx
Outdated
Show resolved
Hide resolved
…/files-ui into feat/sharing-file-browser-1043
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.
Looking great, all is green, life is beautiful, thanks a lot for this huge one.
Created 2 small bug issues to tackle separately.
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.
Left 2 small comments, but looks great otherwise.
The SharesList component seems very much identical to the FilesList except for the menu options. We could potentially hoist the logic of getting these up to context, and just pass it in to the FilesList to avoid having the rest of that repeated.
closes #1043
closes #1157
closes #1162