Skip to content
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

Merged
merged 41 commits into from
Jul 1, 2021
Merged

Sharing file browser #1048

merged 41 commits into from
Jul 1, 2021

Conversation

RyRy79261
Copy link
Contributor

@RyRy79261 RyRy79261 commented May 21, 2021

closes #1043
closes #1157
closes #1162

@RyRy79261 RyRy79261 self-assigned this May 21, 2021
@render
Copy link

render bot commented May 21, 2021

@Tbaut
Copy link
Collaborator

Tbaut commented Jun 15, 2021

We said we'd put this on the side for the time being. Closing so as not to pollute our pulls list.

@Tbaut Tbaut closed this Jun 15, 2021
@Tbaut
Copy link
Collaborator

Tbaut commented Jun 18, 2021

Oops I meant the other one sorry.

@Tbaut Tbaut reopened this Jun 18, 2021
@render
Copy link

render bot commented Jun 18, 2021

@render
Copy link

render bot commented Jun 18, 2021

@CLAassistant
Copy link

CLAassistant commented Jun 18, 2021

CLA assistant check
All committers have signed the CLA.

@lgtm-com
Copy link

lgtm-com bot commented Jun 22, 2021

This pull request introduces 3 alerts when merging 90586f1 into c5c925f - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jun 22, 2021

This pull request introduces 3 alerts when merging e5d9c61 into c5c925f - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jun 22, 2021

This pull request introduces 3 alerts when merging 6fad93e into c5c925f - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jun 22, 2021

This pull request introduces 3 alerts when merging 6dec91e into c5c925f - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@RyRy79261 RyRy79261 added Status: Review Needed 👀 Added to PRs when they need more review and removed Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. labels Jun 24, 2021
@Tbaut Tbaut added the Type: Feature Added to PRs to identify that the change is a new feature. label Jun 24, 2021
Copy link
Collaborator

@Tbaut Tbaut left a 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)

@Tbaut Tbaut added Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. Status: Review Needed 👀 Added to PRs when they need more review and removed Status: Review Needed 👀 Added to PRs when they need more review labels Jun 28, 2021
@Tbaut Tbaut removed the Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. label Jun 30, 2021
@Tbaut Tbaut self-requested a review June 30, 2021 10:34
Copy link
Collaborator

@Tbaut Tbaut left a 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.

Copy link
Contributor

@FSM1 FSM1 left a 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.

@Tbaut Tbaut merged commit 5b562ad into dev Jul 1, 2021
@Tbaut Tbaut deleted the feat/sharing-file-browser-1043 branch July 1, 2021 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review Needed 👀 Added to PRs when they need more review Type: Feature Added to PRs to identify that the change is a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove files from shared folder Upload file to shared folder Shared folder File Browser
5 participants