Skip to content
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

Default templates feature #2

Merged
merged 7 commits into from Jul 5, 2021
Merged

Default templates feature #2

merged 7 commits into from Jul 5, 2021

Conversation

nishantwrp
Copy link
Collaborator

No description provided.

@nishantwrp nishantwrp changed the title Default templates functionality Default templates feature Jun 24, 2021
@nishantwrp nishantwrp marked this pull request as draft June 24, 2021 19:50
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated
if (templateId) {
await joplin.settings.setValue("defaultTodoTemplateId", templateId);
await joplin.views.dialogs.showMessageBox("Default to-do template set successfully!");
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to consider combining this and the above command for code re-usability sake. You could create a function that takes the setting name, and a dialog box message and call it from here and the default template command.

Copy link
Collaborator Author

@nishantwrp nishantwrp Jul 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do this later on. Because the code is repeated just two times and it's only 2 lines of code. Also, that would improve the complexity of the code, so I don't think it's worth doing right now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, you're the owner of this codebase so these calls are ultimately yours.

src/index.ts Outdated
return;
}
}
await joplin.views.dialogs.showMessageBox("No default to-do template is set.");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

src/utils/templates.ts Outdated Show resolved Hide resolved
@nishantwrp
Copy link
Collaborator Author

The functionality is complete. I'll address the review comments asap.

<table>
<tr>
<td><u> Note </u></td>
<td>${noteTemplate ? noteTemplate : "<i>Not set</i>"}</td>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to escape this variable using HTML entities, for example using a lib like this: https://www.npmjs.com/package/html-entities

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Done!

</tr>
<tr>
<td><u> To-do </u></td>
<td>${todoTemplate ? todoTemplate : "<i>Not set</i>"}</td>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, this variable needs to be escaped.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@nishantwrp nishantwrp marked this pull request as ready for review July 3, 2021 10:50
@CalebJohn CalebJohn merged commit fcae7b1 into master Jul 5, 2021
@nishantwrp nishantwrp deleted the default-templates branch July 6, 2021 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants