-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Desktop: Fixes #4891: Solve "Resource Id not provided" error #4943
Desktop: Fixes #4891: Solve "Resource Id not provided" error #4943
Conversation
@@ -53,17 +55,52 @@ function handleCopyToClipboard(options: ContextMenuOptions) { | |||
} | |||
} | |||
|
|||
export function menuItems(): ContextMenuItems { | |||
export async function openItemById(options: any, dispatch: Function) { |
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.
Looking at the code, you only need an ID and an optional hash, so pass this to the function explicitely: itemId: string, dispatch:Function, hash:string = ''
Please do not use "any" as it disables type checking.
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.
Done!
if (!item) throw new Error(`No item with ID ${itemId}`); | ||
|
||
if (item.type_ === BaseModel.TYPE_RESOURCE) { | ||
const localState = await Resource.localState(item); |
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.
Declare the resource here, with type checking: const resource = item as ResourceEntity
and use that resource variable for this block of code.
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.
Done
bridge().showErrorMessageBox(error.message); | ||
} | ||
} else if (item.type_ === BaseModel.TYPE_NOTE) { | ||
let noteItem: any = { |
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's not a "noteItem", it's an action so please call it that.
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.
And type checking on the note: const note = item as NoteEntity
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's not a "noteItem", it's an action so please call it that.
Removed it as options
parameter is no longer needed.
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.
And type checking on the note
const note = item as NoteEntity
Done
if (options.hash) { | ||
noteItem = { ...noteItem, hash: options.hash }; | ||
} |
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.
Make no sense to recreate the object. Just pass the hash directly to the action, as in the original code.
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.
Done
All good now, thank you for the update! |
Fixes: #4891
A function
openItemById
is created insideapp-desktop/gui/NoteEditor/utils/contextMenu.ts
just to avoid duplicate code. TheuseMessageHandler
hook andmenuItems
both use theopenItemById
function so that the user gets the same experience onctrl+click
orright-click+open
a link.Testing
Manual Testing
Manually tested both on markdown editor and the TinyMCE editor( with note-links, resource-links; performing both the operations:
ctrl+click
andright-click+open
).Unit testing
Sorry ( again ), I couldn't find a way to write a unit test for it. Maybe it is possible to add a test, but I think it would be very ( very) complex ( so need some guide ).