Refactor Navigator Card Body for global event listeners #237
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The ContextMenu has nothing to do with the main app, it appears in the `CardBody` or better said DirectoryView. This allows us to later to consolidate the event listeners and if the ContextMenu should be shown to the NavigatorCardBody instead of the app itself. Note: later rename CardBody to DirectoryView.
The context menu handling is Card body specific and does not have to live in the app code.
jelly
force-pushed
the
redo-event-listeners
branch
from
9a35053 to
7bcb584
Compare
jelly
changed the title
Selected item was being passed along to the create directory dialog, which seems to indicate we wanted to support creating a directory in a selected directory. As the UI does not support this nor this is wanted drop it. (Note: icons_cnt also doesn't exists at all)
selectedContext is the same as selected so we can fallback to selected.
The sidebar allowed to delete the currently shown directory which is a bit evil and an easy mistake to make. Don't allow users to make such trivial mistakes.
Simplify deletion by only taking the selected files and the current directory the files are in. Not only does it make the code more clear but it also opens up a way to drop selectedContext.
It's the same as selectedContext but an Array.
When no folder is selected null is passed which crashes the dialog instead pass the currentDirectory object, which apparently does not know about the filetype so the dialog shows "unknown type". This is yet another bug to resolve later.
Just as renameItem, selectedContext is the same as selected except selected is an array.
Same as rename/edit permissions it's the same thing as selectedContext.
This piece of state is now completely replaced by selected and the ContextMenu itself handles when it is visible or not.
cockpituous
reviewed
Feb 1, 2024
Comment on lines
+91
to
+94
| const _copyItem = () => { | ||
| copyItem(setClipboard, selected.length > 1 | ||
| ? selected.map(s => currentDir + s.name) | ||
| : [currentDir + selected[0].name]); |
There was a problem hiding this comment.
These 4 added lines are not executed by any test. Details
| ? selected.map(s => currentDir + s.name) | ||
| : [currentDir + selected[0].name]); | ||
| }; | ||
| const _pasteItem = (targetPath, asSymlink) => pasteItem(clipboard, targetPath.join("/") + "/", asSymlink, addAlert); |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| }; | ||
|
|
||
| if (selected.length === 0) | ||
| return null; |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| // items and not the current path. Refactor this later. | ||
| if (selected.length === 1 && selected[0].name === path[path.length - 1]) { | ||
| menuItems.push( | ||
| { title: _("Paste"), onClick: () => _pasteItem(path, false), isDisabled: clipboard === undefined }, |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| { title: _("Paste"), onClick: () => _pasteItem(path, false), isDisabled: clipboard === undefined }, | ||
| { | ||
| title: _("Paste as symlink"), | ||
| onClick: () => _pasteItem(path, true), |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| ? [ | ||
| { | ||
| title: _("Paste into directory"), | ||
| onClick: () => _pasteItem([...path, selected[0].name], false), |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| { type: "divider" }, | ||
| { title: cockpit.format(_("Delete")), onClick: _deleteItem, className: "pf-m-danger" }, | ||
| ); | ||
| } else if (selected.length > 1) { |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| @@ -222,6 +327,11 @@ | |||
| } | |||
| }; | |||
|
|
|||
| const handleContextMenu = () => { | |||
| const sel = path ? path[path.length - 1] : undefined; | |||
There was a problem hiding this comment.
This added line is not executed by any test. Details
| selected: selected.length === 0 | ||
| ? null | ||
| : selected[0], | ||
| selected: selected[0] || currentDirectory, |
There was a problem hiding this comment.
This added line is not executed by any test. Details
jelly
changed the title
allisonkarlitskaya
approved these changes
Feb 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to move the ContextMenu into the NavigatorCardBody as it has no place in
app.jsxplus remove the context menu related event listeners to the NavigatorCardBody and stop having a per Item event listener for the Context Menu.This is now so far all preparation work, I have a patch to make it use on
onContextMenubut that requires more work to handle the folder switching breaks contextMenu bug.