Desktop: Added support for templates #1647
Conversation
|
Thanks for the PR @CalebJohn, that looks good although indeed we don't have sync if we use files in a directory. That makes me think, I wonder if we should just create a new generic sync item type that would just be a title and a body. That item could be used for any text content we want to sync but that's not a note. Then it can be used for config, template, even scripts, etc. without affecting user notes and notebooks. Basically something that can be built on, while taking advantage of the sync and cross platform functionalities. I'll check how easy it is to add this feature, but if you can think of some drawback let me know. |
|
How would the user edit these? They would connect them to joplin through the config screen? |
|
Yes there would have to be some kind of editor and some way to list the items. |
|
By the way, for the placeholder, dates, etc., do you have a particular format in mind? Personally I always like the Mustache syntax as it's simple and supported by many languages including JavaScript. |
|
I like the {{mustache}} syntax, for now I'm planning to do pretty simple templates, but it would be cool to expose more of the templating features to the user, for now, I plan to just have date and time, And if I (or anyone else) thinks of something else that's useful, I can add it to this pull. |
|
I've added mustache templating (just for date and time) to the new note templates and updated the readme. That concludes all the features that I was planning initially, but I want to wait to see if we get any responses on the forum post I made and I want to do a bit of searching for some example templates that might be nice to link to or host. |
ElectronClient/app/app.js
Outdated
| const templates = await this.loadTemplates(Setting.value('profileDir') + '/templates'); | ||
|
|
||
| this.store().dispatch({ | ||
| type: 'LOAD_TEMPLATES', |
There was a problem hiding this comment.
The action type format is "TYPE_VERB" (type is singular), for example "NOTE_SELECT". So in your case it would be "TEMPLATE_UPDATE_ALL" for consistency with the other actions.
There was a problem hiding this comment.
Thanks for the direction I've changed it to TEMPLATE_UPDATE_ALL
|
Your implementation of custom date/time format is pretty good, as the result is a valid Mustache template (unlike my suggestion which was kind of a hack). As I understand, the template dialog will show up every time you create a new note, is that right? I think we should avoid this extra step if the user doesn't need it. Ideally, I think there should be three menu items to deal with templates:
I guess these three menu items could be under a "Template" menu under "File". The way I see, that would be the minimum we need to make the feature more user friendly, what do you think? As for my suggestion earlier of creating a special sync item, let's leave it out for now. As a first step, we can get the feature pretty much as you've implemented it. Later if needed we can improve it with these special sync items. |
|
I had initially started that way, but a few day's ago changed it so that a prompt would appear in the note editor/viewer area. This prompt is only visible when the note body is empty.
Currently I'm trying to make that prompt a little less ugly... I like your suggestions for menu items, especially in conjuction with the prompt I'm planning. It's smart to go for the extra usability. |
|
I was thinking about this again and I wonder if all the template code should be moved to a separate service. That will encapsulate the template functionalities and allow us to change the loading method and rendering more easily. Later I'd like to use this templating system to allow loading snippets from the Joplin Web Clipper, and then inserting them into pages (at some point I was looking for such a snippet extension and I thought it would be even better if all the snippets were in Joplin, and your system would allow that). So if it's not too much to ask, is there any chance you could move this template code for example to lib/services/TemplateService.js? Then it would just have your |
|
I've done something similar already (I moved all template code into TemplateUtils.js). I'll push up my current wip right away for you to take a look. I'll also start looking into adding it as a service, I'll have more time for this tonight. |
|
I don't quite understand the benefit of using a service rather than just a collection of util functions (what I have started in the latest commit). Is it for forward compatibility? Or is it for the logging maybe? |
|
Utility functions as you've done is fine as well and indeed for now we don't need a service. If we do at some point, it shouldn't be too hard to refactor anyway. |
|
Phew I think this is finally done. Was definitely a bigger change than I anticipated. Please let me know if there is anything that can be changed. That said, I don't really like the look of the prompt that comes up on new notes, if anyone has suggestions to make it look better please let me know! |
CalebJohn
changed the title
|
@CalebJohn, sorry for the lack of feedback on this issue recently. I'm planning to test it properly at some point and get this merged, probably over the weekend. In the meantime thanks for your patience! |
|
No worries, I understand that this is a rather large, non-critical pull. Take your time. |
ElectronClient/app/app.js
Outdated
| @@ -500,6 +503,45 @@ class Application extends BaseApplication { | |||
| screens: ['Main'], | |||
| }); | |||
|
|
|||
| templateItems.push({ | |||
| label: _('Create note from template'), | |||
| visible: fs.pathExistsSync(Setting.value('profileDir') + '/templates'), | |||
There was a problem hiding this comment.
I think this whole block should not be executed (templateItems remains an empty array) if the templates folder does not exist. And we should simply hide or show the root template inside of each individual item. Also please use shim.fsDriver().exists (async) to test for the existence of a file.
There was a problem hiding this comment.
Edit: Sorry I missed that "Open template directory" should always be visible. I guess if you could just cache the value of shim.fsDriver().exists(Setting.value('profileDir') + '/templates') that would be enough. That way the check won't be repeated for each menu item.
There was a problem hiding this comment.
Done! I had to make the entire function async.
| @@ -1021,6 +1026,8 @@ class NoteTextComponent extends React.Component { | |||
| fn = this.commandShowLocalSearch; | |||
| } else if (command.name === 'textCode') { | |||
| fn = this.commandTextCode; | |||
| } else if (command.name === 'insertTemplate') { | |||
| fn = () => { return this.commandTemplate(command.value); }; | |||
There was a problem hiding this comment.
Could you bind commandTemplate to this (in the constructor) so that you can use the same syntax as the other calls?
There was a problem hiding this comment.
The reason for this syntax is because we need to pass command.value to the function. I changed the syntax of the call to fn, let me know what you think.
| let msg = error.message ? error.message : ''; | ||
| msg = 'Could not load template ' + filename + '\n' + msg; | ||
| error.message = msg; | ||
| throw error; |
|
I've left a few comments on the code. Overall there's nothing major to change I think, mostly the file system calls that should go through shim.fsDriver() One issue I've noticed is that when I add my first template, I cannot use it right away without restarting the app. If I add a second template, it doesn't show up in the list of templates either. I'm not sure what would be the solution here: either we watch the templates dir and update templates dynamically; or we provide an menu item to refresh the list of templates. |
|
Thanks @laurent22 I've updated with your suggestions! I think refresh button is the way to go, mostly because I don't really know how to set up a watcher, and ideally this system is just a stop gap until we have a more robust system of syncing templates and styles. |
|
Thanks @CalebJohn, we're nearly there I think. Just a few things remaining:
|
|
All done @laurent22 initially I tried to change the new note prompt to make it fit, but after a few minutes it became clear that you're right, it's not necessary, I've chosen to remove it rather than have it as a toggle. |
|
It's all good now, many thanks for your work @CalebJohn, much appreciated! |
|
the template should also get the filename as the title |
I decided to write the first pass of this exactly the same as userstyle.css (i.e. template files go in the config folder).
The current behaviour is that the user can place any number of markdown (.md) files in
.config/joplin-desktop/templatesand joplin will pick them up. If the user has placed any then whenever they create a new note or todo they will face a prompt to select a template. Leaving the input field blank (or cancelling) will fall back on an empty note. Selecting a template file will automatically populate the body with the contents of that file.While I would like have the syncing functionality in before adding templates. I felt that these should be 2 separate things and templates are a bit easier.
TODO
fixes #1585