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

Add support for template variables #5

Merged
merged 8 commits into from
Jul 26, 2021
Merged

Add support for template variables #5

merged 8 commits into from
Jul 26, 2021

Conversation

nishantwrp
Copy link
Collaborator

No description provided.

@nishantwrp nishantwrp marked this pull request as draft July 14, 2021 19:45
`
<div class="variableInput">
<div class="variableName">
${variable}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just remembered, I need to use encode from html-entities here too.

src/index.ts Outdated
const parsedTemplate = await parser.parseTemplate(template.body);
if (parsedTemplate) {
await joplin.commands.execute("newTodo", parsedTemplate);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I said this before, but when you have identical code duplicated across 5 different places in the codebase, it's probably time to move it in to a function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, got it. Actually, at that time there were only 2 places of repetition, but now this definitely can be refactored. Thanks for pointing it out.

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

);
}

return `<div class="invalidVariable"><i>${variable} has an invalid type.</i></div>`;
Copy link
Member

Choose a reason for hiding this comment

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

Missing encode()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, thanks for pointing it out.

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

`
<h2> Template variables </h2>
<form name="variables">
${variablesFormInputHtml.join("<br />")}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the <br/> are necessary? Since you are giving class names to your variable html, you could add the gap with CSS, which would make for cleaner HTML code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it, will try without <br/>.

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

src/parser.ts Outdated
public async parseTemplate(template: string): Promise<string> {
public async parseTemplate(template: string | null): Promise<string | null> {
if (!template) {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason why the template should be null in this case and not just an empty string? If you just make it an empty string I guess the rest of you code should work, shouldn't it? And then you won't need to have null checks and template: string | null in all the function definitions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

template could be null in a scenario where the user cancels the template selection dialog. That function supports getting different properties of the note like id, body, title etc., I am also planning to support getting complete note object using that function. So, returning an empty string there wrong to me.

Talking about string | null type, If I replace all of these with string. The plugin would still build successfully. I just added string | null so that the developer would know that receiving a null value is possible.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I'm talking about the current version of the function. If you plan to modify it later on, then you'll change the types, input and so on and that's a different topic. For now, as it is, I don't think the function is right.

In your code you are not doing any null check as far as I can see, so you're essentially treating the output of this function as string, not string | null, yet you return null. So again my question is: if you change it to return an empty string, would your code still work? I think it would, and that way you can get rid of the unnecessary string | null.

In general, you should avoid making special cases as it means you'll need to add conditionals to all the code that deal with the output of this function. So if you make it consistently return a string instead, the rest of the code is likely to be simpler too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh okay, got it. I actually misunderstood earlier. Done!

@nishantwrp nishantwrp marked this pull request as ready for review July 23, 2021 19:14
@nishantwrp
Copy link
Collaborator Author

@laurent22 @CalebJohn this pr is ready for review.

);
}

if (type.startsWith("enum(") && type.endsWith(")")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it makes more sense to use the yaml list syntax for this? I think this will affect how your parser works, but should be a lot better for users. For example
This

fruit: enum(apple, orange, banana)

Would become this

fruit:
  - apple
  - orange
  - banana

Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't a requirement for this pull request, but it's something you should think about and perhaps bring to the community.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, in terms of implementation I don't think it's hard to implement. But the way the feature is currently implemented it will create some inconsistency in case we want to add more type of variables in future.

In terms of UX, I want to understand the reason you said

but should be a lot better for users

Because according to me, the users will have to do an equal amount of work in both cases they will have to look to the plugin documentation to see how should they declare enum template variables since it's not a standard.

But, I agree on the fact that as it's something like a list it seems more semantically correct to declare the options in the list syntax.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, I have another idea. Let's create an issue to keep track of this. And in the future, we can support both these syntaxes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think?

My worry is that most users have never heard of an enum, so it's nonsense to them. But a list will make sense to them.

Tracking this in a seperate issue is a good idea, I'm looking forward to hearing your ideas.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, okay. Got it, I always forget that a large fraction of Joplin users are non-programmers and enum is kind of a technical word. Sure, will create an issue for this.

src/parser.ts Outdated
return compiledTemplate(context);
} catch (err) {
console.error("Error in parsing template.", err);
await joplin.views.dialogs.showMessageBox("There was some error parsing this template.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try to make this something actionable for the user. For example

"There was an error parsing this template, please review it and try again."

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

@CalebJohn CalebJohn merged commit c290b78 into master Jul 26, 2021
@nishantwrp nishantwrp deleted the template-variables branch July 27, 2021 10:32
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