-
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
Right click context menu #2134
Right click context menu #2134
Conversation
…t-click-menu-2095
…t-click-menu-2095
Your Render PR Server URL is https://chainsafe-components-stage-pr-2134.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-ca1ro0irrk02be1ur83g. |
Your Render PR Server URL is https://files-ui-stage-pr-2134.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-ca1ro1irrk02be1ur880. |
Your Render PR Server URL is https://storage-ui-stage-pr-2134.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-ca1ro2irrk02be1ur8pg. |
…monorepo into feat/right-click-menu-2095
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.
Amazing job! is working as expected, I tested as well in mobile of course in there you don't have the right click but I checked that all is working correctly there.
PS: I think it will be a good idea to add this to storage as well, maybe not in this ticket but I can create another if you agree
I'm going to add this to storage and reopen this PR for review. |
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 some minor comments in the code. The refactor with the getItemOperations
brings some clarity IMHO which is great, although it's a little messy with the million params 🤷♂️ .
The only main problem I see is that because we don't use the "clever" menu, we end up having an unusable right click menu when we are close to the bottom of the page, and scrolling doesn't help.
bottom.mp4
packages/files-ui/src/Components/Modules/FileBrowsers/views/FileSystemItem/itemOperations.tsx
Outdated
Show resolved
Hide resolved
packages/files-ui/src/Components/Modules/FileBrowsers/views/FilesList.tsx
Outdated
Show resolved
Hide resolved
packages/files-ui/src/Components/Modules/FileBrowsers/views/FilesList.tsx
Outdated
Show resolved
Hide resolved
…monorepo into feat/right-click-menu-2095
I have been able to make the Cleaned up the item operations more and also added to storage. :D |
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.
Love the refactoring and new functionality. I checked mostly Files code, left a couple nits. I did play with Storage as well and noticed that the menu, when it appears is displayed a very brief moment at the top left before being displayed at the right position. See my video
flashing.mp4
packages/files-ui/src/Components/Modules/FileBrowsers/views/FileSystemItem/FileSystemItem.tsx
Outdated
Show resolved
Hide resolved
packages/storage-ui/src/Components/Modules/FileSystemItem/itemOperations.tsx
Outdated
Show resolved
Hide resolved
packages/files-ui/src/Components/Modules/FileBrowsers/views/FilesList.tsx
Outdated
Show resolved
Hide resolved
…t-click-menu-2095
…leSystemItem/FileSystemItem.tsx Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
…Operations.tsx Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
…lesList.tsx Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
…monorepo into feat/right-click-menu-2095
This pull request introduces 1 alert when merging fb883ac into 7071348 - view on LGTM.com new alerts:
|
I was able to see the flash in Firefox a few times. Made some changes to make sure that the dropdown only appears when we have an anchor position. Please check that it works well your side. |
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.
It didn't quite make the cut and was still flashing occasionally. I added some comments to completely solve the issue, with explanations. Let me know if that makes sense, and we should replicate this in storage as well.
Thanks :) :). This was perfect insight into the problem. I've made the updates to files and storage. |
I was still having issue, so I fought a bit and arrived at a state where I really really can't reproduce in this PR #2152 :) |
* some more fixes * cut the needless noise
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.
awesome awesome work, the files view is feeling great thanks to this!
closes #2095
Some notes:
We have a right click(context menu) implementation here. The overall menu system took a refactor. I really wanted to use the MaterialMenu we already had, but for some reason the material menu just does not fire on the second right click. 🤷 I toyed around with it, and then decided to write a simple
AnchorMenu
. some of the behaviour is inspired from drive and dropbox.I didn't add this to storage. If we feel like it should be there, I'll add that in.Added to storage
Submission checklist:
Layout
Theme