Citation popup: display locally stored references #4
Conversation
… to a toolbar button
src/add-bibtex-reference.command.ts
Outdated
|
|
||
| function isFileExisting (file: string): boolean { | ||
| try { | ||
| fs.accessSync(file, fs.constants.F_OK); |
There was a problem hiding this comment.
You should always use the fs async functions, otherwise you freeze the process while the function runs. It might be fine for a few calls here and there but to be future proof it's always best to use async functions.
With promises, the code isn't even more complex - for example here it would be await fs.access().
src/add-bibtex-reference.command.ts
Outdated
| if (!isFileExisting(filePath)) { | ||
| await showMessageBox(constants.ERROR_FILE_NOT_FOUND); | ||
| return; | ||
| } | ||
| if (!isFileReadable(filePath)) { | ||
| await showMessageBox(constants.ERROR_PERMISSION_DENIED); | ||
| return; | ||
| } |
There was a problem hiding this comment.
you don't need any of these checks. Just move the readFile call in the try/catch block (you should do that anyway), and if there's any error you'll catch it there.
There was a problem hiding this comment.
I thought this might be useful for showing descriptive error messages for the user. Am I missing something?
There was a problem hiding this comment.
You don't need that level of detail for something that's very unlikely to happen. But even then you can always give a descriptive error message in the catch block - Could not open file: ${filePath}: ${error.message}
src/ui/citation-popup/index.ts
Outdated
| const ans: string = ( | ||
| `<ul>` + | ||
| refs | ||
| .map(ref => `<li>${ref.title}</li>`) |
There was a problem hiding this comment.
You need to escape the title.
There was a problem hiding this comment.
Sorry, but I don't fully understand what that means. Do you mean that I need to print another attribute of the reference instead of the title ?
There was a problem hiding this comment.
If you add text into HTML, you need to escape it. Otherwise, for example, if the title contains an HTML tag, you'll end up with broken HTML. I usually use this package for this: https://www.npmjs.com/package/html-entities
There was a problem hiding this comment.
Ok, I understand now.
src/ui/settings.ts
Outdated
| label: 'BibTeX File', | ||
| } | ||
| }); | ||
| const options = {}; |
There was a problem hiding this comment.
const options:Record<string, SettingItem>
src/ui/toolbar-button.ts
Outdated
| @@ -0,0 +1,16 @@ | |||
| import joplin from 'api'; | |||
| import { ToolbarButtonLocation } from 'api/types'; | |||
| import constants from '../constants'; | |||
There was a problem hiding this comment.
I don't think it's a good pattern to import all the constants in all the files. If you import only what you need, it will make the code more readable and scalable. import { PLUGIN_TOOLBAR_BUTTON_ID } from '../constants'
|
DONE |
What has been done
addBibTeXReferenceCommand and bind it to a toolbar button.constants.ts.Screenshots
Image of the toolbar button
Image of the citation popup
