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

feat: improve breadcrumbs functionality #1599

Merged
merged 8 commits into from Sep 10, 2020
Merged

Conversation

rafaelramalho19
Copy link
Contributor

@rafaelramalho19 rafaelramalho19 commented Aug 25, 2020

Notes

  • Improves breadcrumbs component.
  • Adds context menu when right clicking a breadcrumb clickable item.

Fixes #840.

@lidel lidel marked this pull request as draft August 25, 2020 15:53
@rafaelramalho19 rafaelramalho19 marked this pull request as ready for review August 31, 2020 16:12
Copy link
Contributor

@jessicaschilling jessicaschilling 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 so awesome 😊 A few notes ...

  1. It looks possible to drag onto a higher-level directory in a breadcrumb (i.e. the hover underline activates) if one is in a subdirectory, but I believe it's actually just trying to import to the current directory. For example, say I have orange.png in files/cats but not in the top level. If I am in files/cats in Web UI and I drag my local copy of orange.png to files in the breadcrumb, I get the warning that A folder with that name already exists even though it doesn't in the top-level directory. (Note as well: that warning message should read 'item' instead of 'folder' to account for situations like this one.)
  2. Alternate take on this: Maybe it's actually just trying to upload to the currently displayed directory instead of the higher directory? (Having a hard time teasing out what's actually going wrong here.)
  3. I believe the right-click menu for the breadcrumbs is acting on the currently displayed directory, too. For example: "Copy CID" for a higher-level breadcrumb item actually copies the CID of the currently displayed directory.
  4. Per Files: icon for parent directory  #840 (comment) - would it be possible to display a directory's CID on hover of the breadcrumb, as in the mockup in that comment? That also is a subtle clue that those items are interactive.
  5. Per Files: icon for parent directory  #840 (comment) - once this is all done, we can remove the .. folder from the main file grid entirely (that was what kicked off this whole exercise, ha ...)

Thank you!

@jessicaschilling
Copy link
Contributor

Updated Joyride tour in 91a0d3a - screenshot below:
image

Copy link
Contributor

@jessicaschilling jessicaschilling left a comment

Choose a reason for hiding this comment

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

The following right-click actions for breadcrumbs seem to work perfectly 😊

  • Share link
  • Copy CID
  • Inspect
  • Rename
  • Delete

The following right-click actions don't quite work:

  • Pin:
    • If a directory is un-pinned, right-clicking its breadcrumb and selecting "Pin" pins it successfully
    • If a directory is already pinned, right-clicking breadcrumb still says "Pin" (not "Unpin")
  • Download: This downloads a .txt file that actually contains (valid) dir-index-html HTML for displaying the directory! 🤣 Voting that we should just remove this option entirely from breadcrumbs (it doesn't appear when right-clicking a directory in the files table).

Issues with moving files via dragging:

Issues with importing files via dragging:

  • I can drag a file into the main file list (not the breadcrumb) when I am in the home directory, or home+1 directory (e.g. home/cats), but not any deeper (e.g. not home/cats/moar cats or home/cats/moar cats/even moar cats) -- it gives "folder with that name already exists" error. (Using the "Import" button in any directory works just fine.)
  • Dragging onto any level of breadcrumb actually just imports it into the presently displayed directory.

Two more things from earlier:

  • Per Files: icon for parent directory  #840 (comment) - would it be possible to display a directory's CID on hover of the breadcrumb, as in the mockup in that comment? That also is a subtle clue that those items are interactive.
  • We're using folderExists (line 7 in public/locales/foo/notify.json) for the error whenever a user tries to import a file with an already existing name, too. Should we just make this more general ("an item with that name already exists") or should we add a separate itemExists i18n key?

Thank you so much for continuing to untangle this 😊

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Love the improved breadcrumbs! Before we merge, need to address what @jessicaschilling said +

  • Download: as @jessicaschilling noted, either remove it or fix (I'd vote fix: wire it up to download TAR archive using the same code path as one when a custom directory is created when multiple items are marked with checkboxes and Download is clicked on the bottom drawer)

  • Should we just make this more general ("an item with that name already exists") or should we add a separate itemExists i18n key?

    I vote for making it generic, that's what we did in other places.

  • 💫 breadcrumb for parent has all the context menu items, but its parents only have three items
    – bit confusing, are we able to unify this?

  • 🐛 Found a D&D bug: while in "Files, click on the "Files" in navbar and keep it clicked, then drag it into Files ;-) – Unhandled Rejection (TypeError): entry is null is produced

  • 🐛 When I right click on a crumb, its menu is displayed. When I click on another, its menu does not display: I see menu for previous crumb + browser-native menu instead. Was able to reproduce in both Firefox and Chromium. Are we able to close existing menu if a different crumb is right-clicked?

  • 🐛 💫 dragging file item onto the crumb of parent directory SOMETIMES copies it instead of moving
    I've seen Files.mv works different according to target parent directory kubo#7646 but was not able to reproduce it in CLI, then I had it happen once for me in WebUI while testing this PR, but then worked ok.. unclear is its a bug in webui (old dir listing being cached) or ephemeral go-ipfs bug (sometimes not refreshing MFS root CID?). It may be tricky to debug, so as this is just a mild annoyance, and not something that breaks things or lose data, I would not block this PR, and fill separate issue for tracking this.

@rafaelramalho19
Copy link
Contributor Author

  • Unpin label in context menu

  • Download in a directory - this was actually the assumption that I made that the last item of the breadcrumbs would be always a file 🤦 Sorry about that f(a)il(e), changed the method to use ipfs.files.stat to get both CID and Type

  • Dragging onto any level of breadcrumb actually just imports it into the presently displayed directory. - I've updated the code from the dragging already uploaded files and forgot to update the one from new files 🤦

  • @jessicaschilling "would it be possible to display a directory's CID on hover of the breadcrumb, as in the mockup in that comment?" That would force us to run ipfs.files.stat for every breadcrumb. We could make this async (after loading with a small delay), but don't you think that the cursor: pointer is sufficient feedback for the user? I really don't want to impact the normal usage of the files page because of a small interaction 🤔

  • Changed translation to "an item", makes more sense 👍

breadcrumb for parent has all the context menu items, but its parents only have three items – bit confusing, are we able to unify this?

The logic is that if an item is a directory, you can only do the 3 options, if it's a file you can do the full list

  • Found a D&D bug: while in "Files, click on the "Files" in navbar and keep it clicked, then drag it into Files ;-) . Fixed every drag & drop of text inside webui. If you find a weird use-case where this should be possible please tell me

When I right click on a crumb, its menu is displayed. When I click on another, its menu does not display: I see menu for previous crumb + browser-native menu instead. Was able to reproduce in both Firefox and Chromium. Are we able to close existing menu if a different crumb is right-clicked?

@lidel unfortunately I don't think it's possible with the current solution, since we create an invisible overlay around the dropdown to catch mouse interactions. There's 2 solutions that come to my head:

  • Remove the overlay and change the click handling to a global listener that checks if there was a click the dropdown and just close it if that's the case.
  • Make a onContextMenu handler on the overlay that simply prevents the default behavior (so that you don't see the native right click action)

What option would you prefer?

--

Thanks both of you for your deep review 🙏

@jessicaschilling
Copy link
Contributor

Bummer about the hover being expensive. We can leave it out.

@lidel
Copy link
Member

lidel commented Sep 9, 2020

@rafaelramalho19 IIUC going with "global listener that checks if there was a click the dropdown and just close it if that's the case" would produce better UX:

  • We want to disable native right click action and only display webui ones when interacting with breadcrumbs or file list
  • There should be only one context menu at a time
    • if a new one is opened an existing one should be automatically closed

I rely on your judgement here. If this is a 🕳️ 🐇 and not something you can quickly do, I suggest we ship v2.11 without any context actions on breadcrumbs (its still better than before this PR), and fill separate issue for adding them later 👍

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@rafaelramalho19 nearly there, probably the last nit: dragging image onto breadcrumb breaks the app – mind applying simililar fix as you did for strings and navbar?

Copy link
Contributor

@jessicaschilling jessicaschilling left a comment

Choose a reason for hiding this comment

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

Quick note on the Joyride helptext, otherwise I'm good whenever @lidel is 😊

public/locales/en/files.json Show resolved Hide resolved
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@rafaelramalho19 drag-and-drop works great now, but recent commit introduced regression in how PDF and videos are rendered (images are fine, so we missed it).

before they took entire free space, now they are a narrow column:

before now
ff2-2020-09-09--18-38-26 ff-2020-09-09--18-34-08

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@rafaelramalho19 thanks, PDF previews look fine again.

Q: you mentioned that we should keep joyride for them, buy I've rebuilt this PR and now I get no context menu on breadcrumbs at all. Mind checking on your end? nvm, it was due to HTTP API I used for testing being slow to respond due to high number of Pins I have in local repo

src/files/file-preview/FilePreview.js Outdated Show resolved Hide resolved
src/files/file-preview/FilePreview.js Outdated Show resolved Hide resolved
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

LGTM. Let's merge?

ps. The slowdown I experienced is not related to this PR, but was already present in go-ipfs.
I filled #1618 to track it.

@rafaelramalho19 rafaelramalho19 merged commit d0ab66d into master Sep 10, 2020
@rafaelramalho19 rafaelramalho19 deleted the feat/super-breadcrumbs branch September 10, 2020 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Files: icon for parent directory
3 participants