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

DnD with breadcrumbs #1836

Merged
merged 25 commits into from
Jan 11, 2022
Merged

DnD with breadcrumbs #1836

merged 25 commits into from
Jan 11, 2022

Conversation

tanmoyAtb
Copy link
Contributor

@tanmoyAtb tanmoyAtb commented Dec 25, 2021

closes #1772


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 Dec 25, 2021

@render
Copy link

render bot commented Dec 25, 2021

@render
Copy link

render bot commented Dec 25, 2021

@github-actions github-actions bot added the Type: Feature Added to PRs to identify that the change is a new feature. label Dec 25, 2021
@tanmoyAtb tanmoyAtb changed the title readcrum refs and active colors DnD with breadcrumbs Dec 25, 2021
@tanmoyAtb tanmoyAtb marked this pull request as ready for review December 25, 2021 17:32
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.

I didn't check the code yet, but testing the functionality, I got an error from the mv from the api.
I have a file in /some/folder and drag it to move it to /some. The request fails with

Object { code: 400, message: "paths is required" }
The request is as follow:

{"paths":[],"new_path":"/some/"}

@tanmoyAtb
Copy link
Contributor Author

I didn't check the code yet, but testing the functionality, I got an error from the mv from the api. I have a file in /some/folder and drag it to move it to /some. The request fails with

Object { code: 400, message: "paths is required" }
The request is as follow:

{"paths":[],"new_path":"/some/"}

Its working my side, I even tried with the same /some/folder tree. Can you check if the drag and drop on a folder in the file browser lists work instead of the breadcrumb ?

@tanmoyAtb tanmoyAtb requested review from FSM1 and asnaith January 3, 2022 16:43
@Tbaut
Copy link
Collaborator

Tbaut commented Jan 3, 2022

There are some weirdnesses.. I moved my file to the /some/folder folder, then try to DnD with breadcrumb, it fails (not on the video), I try again (visible in the video), it works, I browse to /some, drag the file down to /some/folder, it works, then try to drag it up using the breadcrumb, it fails again..

weirdness.mp4

@tanmoyAtb
Copy link
Contributor Author

There are some weirdnesses.. I moved my file to the /some/folder folder, then try to DnD with breadcrumb, it fails (not on the video), I try again (visible in the video), it works, I browse to /some, drag the file down to /some/folder, it works, then try to drag it up using the breadcrumb, it fails again..

weirdness.mp4

So I think there's some sensitivity around the way we are dragging. If you check the clip, when the move failed, there was no drag preview. Somehow the drag event failed to grab the source.

I'll spend some time to see if I can find the actual issue.

@tanmoyAtb
Copy link
Contributor Author

Found the issue, The useDrag wasn't re rendering when file contents changed by moving into a folder. It should be good to go now.

@tanmoyAtb tanmoyAtb requested a review from Tbaut January 5, 2022 18:24
@asnaith
Copy link
Member

asnaith commented Jan 5, 2022

Hey Tanmoy, I'm seeing some strange behaviour but it's kind of difficult to describe some of them. I've captured them on video so hopefully that helps.

  • I noticed there's an issue with the appearance of the breadcrumb folders after a browser refresh
breadcrumb.issue.after.refresh.mov
  • When you drag a file to the folder on the breadcrumb that it's already residing in it will append a (1) to the file name and also the modal is cut short
Moving.file.into.current.directory.mov
  • When you try a regular drag and drop between folders (non breadcrumb) I've noticed the move success toast elongates to the full screen (but I think it's related to these changes as I can't reproduce this on stage).
Elongated.modal.DND.non.breadcrumb.mov
  • I managed to somehow get the folder name to appear in the blue element that appeared at the bottom of the screen.
blue.element.weirdness.mov

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 6, 2022

There are weird CSS quirks indeed in the render built preview, but not when you build the branch locally (also there's no funky animation). I cleared the cache from render.com and redeployed. Should be live soon.

I can't reproduce the issue I had before, and from what Andrew saw, I can see the issue with moving to the folder it's already in, we should prevent this if possible

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.

I won't lie, the useDrop is pretty magical to me still as I've never played with it. My comments on the code are therefore on everything else :D

packages/common-components/src/Breadcrumb/Breadcrumb.tsx Outdated Show resolved Hide resolved
packages/common-components/src/Breadcrumb/Breadcrumb.tsx Outdated Show resolved Hide resolved
packages/common-components/src/Breadcrumb/Breadcrumb.tsx Outdated Show resolved Hide resolved
packages/common-components/src/Breadcrumb/Breadcrumb.tsx Outdated Show resolved Hide resolved
packages/common-components/src/Breadcrumb/Breadcrumb.tsx Outdated Show resolved Hide resolved
packages/files-ui/src/Utils/pathUtils.ts Outdated Show resolved Hide resolved
tanmoyAtb and others added 4 commits January 7, 2022 00:15
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
@tanmoyAtb
Copy link
Contributor Author

tanmoyAtb commented Jan 6, 2022

Alright I've updated The current folder in breadcrumb to not accept drops!

But the breadcrumbs breaking up is a serious issue. Look at this one. This happens when you build it even locally.

Screenshot 2022-01-07 at 12 14 30 AM

!

What happens is JSS re initialises classnames to the dynamic component I'm passing as component to a crumb.
The passed in breadcrumbs have jss1 and jss2 which are existing classnames which are injecting their styles !!!!!

I found this thread on the jss repo.
cssinjs/jss#1458

A couple of hacky solutions in my mind i'll need to check.

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 7, 2022

Wow this is wild. One thing that crossed my mind and you prob thought about, what if we prevent the classes from having generated names, and actually go with our own naming like: className={clsx(classes.duplicated, "unique-identifier")} and then

{
  duplicated: {
    "&.unique-identifier" : {
      ...
    }
  }
}

@tanmoyAtb
Copy link
Contributor Author

Wow this is wild. One thing that crossed my mind and you prob thought about, what if we prevent the classes from having generated names, and actually go with our own naming like: className={clsx(classes.duplicated, "unique-identifier")} and then

{
  duplicated: {
    "&.unique-identifier" : {
      ...
    }
  }
}

This would have worked, Yes !
But I found the solution in the material docs:
https://mui.com/styles/api/
mui/material-ui#11843

The PR seems to be in order now !

@tanmoyAtb tanmoyAtb requested a review from Tbaut January 8, 2022 16:57
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.

Played again with it, including moving and uploading files, and it worked like a charm 🎉
The code looks neat too, just left a small comment for a nit.

…lesList.tsx

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

asnaith commented Jan 10, 2022

Awesome work Tanmoy, I can't see any of the previous weirdness and the feature is working great for me too

@Tbaut Tbaut merged commit 05ada0a into dev Jan 11, 2022
@Tbaut Tbaut deleted the feat/tanmoy-breadcrumbs-as-dropzone-1772 branch January 11, 2022 10:17
@FSM1 FSM1 mentioned this pull request Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

File Browser Breadcrumbs as Dropzone Targets
4 participants