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

Always ask for the note title when creating from template #645

Merged
merged 2 commits into from
May 22, 2021

Conversation

movermeyer
Copy link
Collaborator

@movermeyer movermeyer commented May 20, 2021

What are you trying to accomplish?

In order to create a note from a template you need two things:

  1. the filepath
  2. the file contents.

In cases where the template doesn't explicitly specify the path using the metadata syntax (i.e. all current templates since the metadata syntax isn't implemented yet), we'll add $FOAM_TITLE to the list of variables to resolve, and use it as the filepath.

This will prompt the user for a title as they (reasonably) expect it to.

Then in the future when the template metadata block contains an explicit override of the filepath, we will stop adding $FOAM_TITLE, and whether or not the user gets prompted will depend on the contents of the filepath metadata.

Fixes #640
Ref: #640 (comment)

What approach did you choose and why?

I've added FOAM_TITLE unconditionally to the list of variables to resolve, resulting in Foam: Create New Note always asking for a note's title.

In the future, we'll make this conditional on whether the template provides the filepath to use in its metadata block.

What should reviewers focus on?

Are there cases you could imagine where someone would not want it to prompt for a title, besides the future scenario where the filepath metadata is provided in the template?

I'm writing this very early in the morning with little sleep. I may be forgetting some case???

What doesn't change with this PR?

Note: This PR does not change the behaviour of Foam: Create New Note From Template.
In cases where the template does not reference ${FOAM_TITLE}, the user will still not be prompted to enter a title.
This works, since in Foam: Create New Note From Template there is a separate step where they fill in the filepath they desire. If we were to always resolve ${FOAM_TITLE}, it could make this command take longer than necessary, especially in cases where their desired filepath does not match the desired title:

Example of where always asking for title in Foam: Create New Note From Template would be a worse UX

  1. User runs Foam: Create New Note From Template; selects an empty template.
  2. User is prompted for a title; enters "On the Origin of Species"
  3. Filepath prompt now has <dir>/On the Origin of Species.md
  4. User deletes the existing filepath prompt and changes it to <dir>/draft-1.md

The user never wanted the title to be reflected in the filepath, so there was no need to prompt them.
It's fine to always prompt them in the Foam: Create New Note case, since that command is opinionated in order to have the most streamlined command for the most common use case. Foam: Create New Note From Template is slower, but less opinionated, and more flexible/powerful.

@movermeyer movermeyer marked this pull request as ready for review May 20, 2021 08:10
@riccardoferretti
Copy link
Collaborator

I think I understand the use case you are describing, I am wondering if the approach we should take could be along the lines of:

  1. if no $FOAM_TITLE is used in the template, default to New Note
  2. if no filepath property is defined by the template, ask for it and default to <current_dir>/$FOAM_TITLE

in this way, if the $FOAM_TITLE is in the template, we will ask the user, and use it for default in the path (the flow you described). If instead the title is not needed to fill in the template, we skip one step by only asking for the filepath.

I believe the issue you linked to would also be addressed by this flow.

What do you think?

p.s. completely unrelated, but I really like the PR template you use, I would like to make it the standard PR for Foam

In the future, we'll make this conditional on whether the template provides the filepath to use in its metadata block
@movermeyer
Copy link
Collaborator Author

What do you think?

@riccardoferretti (I hope I've interpreted it correctly 🤞)

I've tried to map out the behaviour more clearly so that we can be sure that we are talking about the same things.

There are 5 cases, which I have labeled A-E. I believe that the only difference between the behaviours is in Case D.

Current behaviour on master branch

filepath doesn't exist filepath exists, but doesn't contain $FOAM_TITLE filepath exists, and contains $FOAM_TITLE
$FOAM_TITLE in the template A: 1 step: Ask for title B: 1 step: Ask for title C: 1 step: Ask for title
$FOAM_TITLE not in the template D: 0 steps: New Note.md created E: 0 steps: note created according to filepath N/A, if $FOAM_TITLE is in the filepath, it is in the template

Current behaviour on this PR

This PR changes the behaviour of Case D to ask for the title. This is preferred over the existing master-branch behaviour since always having to rename New Note.md to the desired name (outside of the note creation flow) is tiresome.

filepath doesn't exist filepath exists, but doesn't contain $FOAM_TITLE filepath exists, and contains $FOAM_TITLE
$FOAM_TITLE in the template A: 1 step: Ask for title B: 1 step: Ask for title C: 1 step: Ask for title
$FOAM_TITLE not in the template D: 1 step: Ask for title E: 0 steps: note created according to filepath N/A, if $FOAM_TITLE is in the filepath, it is in the template

Behaviour described in comment proposal

If I'm parsing it correctly, Case D is once again the only thing that changes:

filepath doesn't exist filepath exists, but doesn't contain $FOAM_TITLE filepath exists, and contains $FOAM_TITLE
$FOAM_TITLE in the template A: 1 step: Ask for title B: 1 step: Ask for title C: 1 step: Ask for title
$FOAM_TITLE not in the template D: 1 step: Ask for path (Showing <current_dir>/New Note.md) E: 0 steps: note created according to filepath N/A, if $FOAM_TITLE is in the filepath, it is in the template

we skip one step by only asking for the filepath

Both this PR and the comment proposal have the same number of steps to create a note. No steps are skipped?

What do you think?

I think the only difference between these is what to ask the user for when a template does not include $FOAM_TITLE?:

  • In this PR, it asks the user for the title of the note
  • In the comment proposal, it asks the user for the path of the note

I prefer asking the user for the title over the path, for two reasons:

  • Consistency: In this PR, Foam: Create New Note only ever asks for a single thing in all cases: the title
    • In the comment proposal, Case D will ask for a path, unlike Cases A-C which ask for a title
  • Aesthetic: In general, I'd rather that I never have to look at paths. Titles look cleaner / nicer.
    • If you need control over the path, you can specify a filepath (once implemented) or fall back to use Foam: Create New Note From Template

@riccardoferretti
Copy link
Collaborator

hi @movermeyer, in your example above:

  1. User runs Foam: Create New Note From Template; selects an empty template.
  2. User is prompted for a title; enters "On the Origin of Species"
  3. Filepath prompt now has /On the Origin of Species.md
  4. User deletes the existing filepath prompt and changes it to /draft-1.md

There are two inputs required from the user, wasn't this the flow you were thinking of?

@movermeyer
Copy link
Collaborator Author

movermeyer commented May 21, 2021

There are two inputs required from the user, wasn't this the flow you were thinking of?

@riccardoferretti That example in the PR description is for Foam: Create New Note From Template, to demonstrate why I'm not also changing the behaviour of that flow in this PR. The code paths look very similar at first glance, so a reviewer might have asked "Why aren't we also making this change for Foam: Create New Note From Template?". I've updated the PR description to hopefully be a little more clear.

I think your comment describes the current behaviour of Foam: Create New Note From Template: We only ask for the title if $FOAM_TITLE is needed.

Current behaviour of Foam: Create New Note From Template:

  1. User selects the template
  2. If $FOAM_TITLE is needed, ask for the title
  3. Confirm the filepath (using $FOAM_TITLE if it was resolved, else New Note as the default)

So currently, there are at most 3 steps to Foam: Create New Note From Template, but one of them is conditional on $FOAM_TITLE use. Making the same change to Foam: Create New Note From Template that this PR makes to Foam: Create New Notewould cause it to always be 3 steps, which is not what we want.

@riccardoferretti
Copy link
Collaborator

oops, my wires crossed on that one, you are right

Copy link
Collaborator

@riccardoferretti riccardoferretti left a comment

Choose a reason for hiding this comment

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

LGTM!

@riccardoferretti riccardoferretti merged commit 1cb8174 into foambubble:master May 22, 2021
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.

Create New Note Skips Asking Filename
2 participants