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

New sharing design #1927

Merged
merged 27 commits into from
Feb 11, 2022
Merged

New sharing design #1927

merged 27 commits into from
Feb 11, 2022

Conversation

tanmoyAtb
Copy link
Contributor

closes #1908


Submission checklist:

Layout

  • Change looks good in the desktop web ui
  • Change looks good in the mobile web ui

Theme

  • Components / elements inspected in light mode
  • Components / elements inspected in dark mode

@render
Copy link

render bot commented Feb 8, 2022

@render
Copy link

render bot commented Feb 8, 2022

@render
Copy link

render bot commented Feb 8, 2022

@github-actions github-actions bot added the Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes. label Feb 8, 2022
@tanmoyAtb tanmoyAtb requested review from Tbaut, RyRy79261, asnaith and FSM1 and removed request for RyRy79261 February 8, 2022 19:52
@Tbaut
Copy link
Collaborator

Tbaut commented Feb 9, 2022

Can you check the tests @tanmoyAtb Link and file sharing are legitimately failing

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 very nice and working very well. I left a couple comments, the CreateOrManage... file is slowly getting out of hands with 650+ lines of codes. I wish we could some code to components (like each user line) to some lower-level component, passing around some props. This could be left for a refactoring later on. I see a lot of reader and writer specific code, although we could have somewhat generic functions handling both. and reducing a little the JSX, as it's barely readable (and don't get me wrong, it was already in a bad shape, this PR is not the 100% guilty one).

In any case, beside my comments below, we can leave that for a later refactoring.

tanmoyAtb and others added 2 commits February 9, 2022 20:57
…ManageSharedFolder.tsx

Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
@tanmoyAtb
Copy link
Contributor Author

This is very nice and working very well. I left a couple comments, the CreateOrManage... file is slowly getting out of hands with 650+ lines of codes. I wish we could some code to components (like each user line) to some lower-level component, passing around some props. This could be left for a refactoring later on. I see a lot of reader and writer specific code, although we could have somewhat generic functions handling both. and reducing a little the JSX, as it's barely readable (and don't get me wrong, it was already in a bad shape, this PR is not the 100% guilty one).

In any case, beside my comments below, we can leave that for a later refactoring.

I agree with all of the points, I felt the file getting out of hand as well. I have some refactoring in mind that I'll add in this PR. The readers and writers part is something we had before, I'll see about merging them as well. Checking on the tests as well.

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.

Great stuff @tanmoyAtb 🥇

Copy link
Member

@asnaith asnaith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @tanmoyAtb! Nice test fixes too @FSM1 👍

I made some minor suggestions but mainly requested a change to put loginData back to how it was before.

FSM1 and others added 2 commits February 11, 2022 12:00
Co-authored-by: Andrew Snaith <asnaith@users.noreply.github.com>
Co-authored-by: Andrew Snaith <asnaith@users.noreply.github.com>
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.

Great refactor, thanks a lot!
there are just some small nits found by Andrew, and we can merge 🎉

@FSM1 FSM1 enabled auto-merge (squash) February 11, 2022 14:20
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.

🚀

Merging this one right away to see if creates conflicts with others

@Tbaut Tbaut requested a review from asnaith February 11, 2022 14:52
@FSM1 FSM1 merged commit a3b9963 into dev Feb 11, 2022
@FSM1 FSM1 deleted the mnt/new-share-design-1908 branch February 11, 2022 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Sharing design
5 participants