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

#3215 Adds ability to create template from doctype editor #3366

Merged
merged 3 commits into from
Oct 23, 2018
Merged

#3215 Adds ability to create template from doctype editor #3366

merged 3 commits into from
Oct 23, 2018

Conversation

skttl
Copy link
Contributor

@skttl skttl commented Oct 20, 2018

Prerequisites

Description

I added a button for creating a new template in the template view of the doctype editor. The button uses the template resource to fetch a template scaffold, and then save a new template. After saving, the template gets selected as allowed, and if there is no default template selected, the new template will also be the default.

https://i.imgur.com/KQbmjGH.gifv

@poornimanayar
Copy link
Contributor

Hi @skttl ,

Thank you for the PR. I shall give your PR a spin and shall let you know.

I am going to try and test it for the following scenarios

  1. Adding an extra template for a doc type which already has a default
  2. Creating a template for a doc type which does not have one yet.

If there are any cases you can think of let me know.

Regards,
Poornima, Community Pull Request team

@poornimanayar
Copy link
Contributor

Hey @skttl ,

I have given the functionality a quick test. For a doc type with no default template it adds a new template and uses that as default. But when I try to add a new template to a doc type which already has a default one it creates a template and adds it as well which is fine but then I have got 2 templates with the same name and I think it could be confusing for someone who is doing it for the first time and not quite thorough with the functionality. Do you think you can append a "(1)" to the end of the template to make it more clear? Below is the screenshot for this.

image

Also I noticed that once you add a new template and click away from the doc type, the new template still remains in the tree and I am wondering whether this could lead to a situation where number of templates are created and left there doing nothing. One thing I can think of it to show the notification to say "Are you sure..?" but thats not something the templates screen does even otherwise when you choose another template and move away.

@nul800sebastiaan thoughts on all of the above?

Poornima

@skttl
Copy link
Contributor Author

skttl commented Oct 23, 2018

Hi @poornimanayar

Thanks for testing :)

The template tree also allows for templates to have the same name, and I think this should have the same. I see what you mean though.

Regarding clicking away and not saving, this is another issue (#3368) which is already fixed by @kjac in #3372

Copy link
Member

@nul800sebastiaan nul800sebastiaan left a comment

Choose a reason for hiding this comment

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

Hey @skttl! While we think it is really cool to have a button here, @poornimanayar has already discovered the flaw in this: what if there already is a template?

We can accept this PR if you show the button only if you do NOT yet have a template for a document type.
We actually don't want to encourage people having multiple templates on a single document type, it should be something rare. Adding a button to add more templates will just give people an incentive to create a template without thinking about it.

Can you update this PR with that requirement in mind please?

@skttl
Copy link
Contributor Author

skttl commented Oct 23, 2018

Sure, here you go :)

image
image
image

@poornimanayar
Copy link
Contributor

Hey @skttl ,

I have tested this again and it works as advertised :-) Happy to approve this and thank you for the nice little feature!

Poornima

@nul800sebastiaan nul800sebastiaan self-assigned this Oct 23, 2018
Co-Authored-By: skttl <snielsen43@gmail.com>
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.

3 participants