NotePlan / plugins

The main NotePlan repository, which contains the source code for all NotePlan plugins. If a release entry has been created, it will be available for installation from within NotePlan application.
MIT License
168 stars 58 forks source link

Fix up pickDate() for Templating #269

Closed jgclark closed 2 years ago

jgclark commented 2 years ago

Here's the start of the template definition:

---
title: School Session Template
type: empty-note
---
### <%- prompt('place','Which school/group?',['GBP','FLJ','FLI','Cubs']) %> <%- prompt('sessionType','What sort of session is this?',['assembly','service','lesson']) %>, <%= pickDate('question','Date of session') %>
* decide talk point {-7d} 10:00-12:00
...

This runs OK, except that:

  1. Before the date picker appears, it pops up this error message 'Invalid JSON5 in your configuration ...' -- i.e. there's some dependency still here with old nmn.Templates
  2. It doesn't embed the given text in the note but instead writes '[object Promise]'
  3. I think when I tried it with the pickDate as the first tag it asked the question last ... so possibly something odd with an await?

I've had a look but its not something I can quickly diagnose.

However, before diving in, let's just see work out what's best to do with pickDate, as I think it could do with some work to bring it in line with the rest of Templating.

  1. Should it be renamed promptDate()?
  2. It's currently picking up code from helpers/userInput(), but would it be better to separate that off, and have this as a templates-only thing?
  3. I hope in time that Eduard gives us a 'native' date picker UI element, which could then be used instead in the implementation of this.

Thanks.

mikeerickson commented 2 years ago

Hello

You call to

<%= pickDate('question','Date of session') %>

should be in the form

<%- pickDate({'question':'Date of session'}) %>

as the pickDate method expects an object and you access question from an object in the datePicker method (line 309)

CleanShot 2022-04-22 at 07 57 43

If you change to the form I have outlined above, every works as expected

mikeerickson commented 2 years ago

@jgclark Here is a legacy version of a template one of you guys supplied, which shows the question key in object form

# # Date Tester template (legacy)
---
(from @dwertheimer; see issue #74)

## Today:

8601 date the hard way:
- {{currentDate({locale: 'sv-SE', dateStyle: 'short'}) }}

Formatted:
- {{ formattedDateTime('%Y-%m-%d %I:%M:%S %P') }}

8601 the easy way:
- {{ date8601() }}

pickdate:
- {{ pickDate() }}
- {{ pickDate({question:'Please enter a date:'}) }}
- {{ pickDateInterval() }}
- {{pickDateInterval({question:'Date interval to use:'}) }}

date:
{{ date({locale: 'sv-SE', dateStyle: 'short'}) }}
mikeerickson commented 2 years ago

@jgclark Here is the point in userInput.js which I was referring to

https://github.com/NotePlan/plugins/blob/85dc7aa9a58ec083869f48914c6ac8df334bd838/helpers/userInput.js#L309

jgclark commented 2 years ago

Thanks for finding the issue in my syntax. I'm going to re-open this, as my wider questions and suggestions still stand, and there is documentation to improve too.

It seems to be inconsistent with how prompt() is called: <%- pickDate({'question':'Date of session'}) %> versus <%- prompt('place','Which school/group?') %> so no wonder I didn't get the syntax right.

I'm also happy to rename it to promptDate().

Looking at the only mention in the Templating docs, I see it's part of the page Mike and I improved but didn't finish. In particular I posed the question whether pickDate() and pickDateInterval() functions should become part of the system module, or a new helpers module.

Thoughts?

mikeerickson commented 2 years ago

@jgclark I have not documented pickDate as it was a carry over from previous plugin, I just added it so things could still work without refactoring end user templates.

The "date" prompt could be converted to

<% prompt('startDate','Please enter a date:') %>

I am not sure what direction you would like to go with this?

Another option would be to change datePicker from userInput.js to use

CommandBar.textPrompt(...)

jgclark commented 2 years ago

@jgclark I have not documented pickDate as it was a carry over from previous plugin, I just added it so things could still work without refactoring end user templates.

Which is fine. I'm now trying to tidy up a few loose ends now that the important stuff is released.

The "date" prompt could be converted to

<% prompt('startDate','Please enter a date:') %>

Sorry to be dense, but how does this then know to put up a date picker versus the standard field? The reason I wrote the date picker was to be able to check that a valid date had been returned. And that in time it could be replaced with something more specific by EM.

I am not sure what direction you would like to go with this?

Another option would be to change datePicker from userInput.js to use

CommandBar.textPrompt(...)

~My reason for not shifting the underlying helper function to use the newer textPrompt was that it doesn't give a way AFAICT to put default or hint text in the input field. This is what I do with the earlier CommandBar version which shows 'YYYY-MM-DD' as a hint that it needs to be the ISO form.~

mikeerickson commented 2 years ago

You can do a default like this

const value = await CommandBar.textPrompt('Enter Date', '', 'defaultValue')

I have also exposed through prompt command

<%- prompt('placeholder','promptMesage','default') %>

If the default value is an array of choices, it will show the CommandBar.showOptions, otherwise uses CommandBar.textPrompt

See the plugin documentation

/**
* Note: Available from v3.3.2
* Show a native text input prompt to the user with a title and a message text. The buttons will be automatically "OK" and "Cancel". You can supply a default text which will be prefilled. If the user hits "Cancel", the promise returns false.
* @param {String} 
* @param (String)
* @param (String?)
* @return {Promise<Bool | String>}
*/
.textPrompt(title, message, defaultText)

or in the .flow-typed file you have been maintaining

  /**
   * Note: Available from v3.3.2
   * Show a native text input prompt to the user with title and message text.
   * The buttons will be "OK" and "Cancel".
   * You can supply a default value which will be pre-filled.
   * If the user selects "OK", the promise returns users entered value
   * If the user selects "Cancel", the promise returns false.
   * @param {String}
   * @param {String?}
   * @param {String?}
   */
  textPrompt(title: string, message: string, defaultValue: string): Promise<string | boolean>,
}
mikeerickson commented 2 years ago

My reason for not shifting the underlying helper function to use the newer textPrompt was that it doesn't give a way AFAICT to put default or hint text in the input field. This is what I do with the earlier CommandBar version which shows 'YYYY-MM-DD' as a hint that it needs to be the ISO form.

Example to refactor datePicker to use something like

const value = await CommandBar.textPrompt('Enter start date','','YYYY-MM-DD') 
mikeerickson commented 2 years ago

@jgclark So, just let me know how you want to handle this and I can adjust templating implementation accordingly.

jgclark commented 2 years ago

I've updated the userInput.js accordingly, now you corrected my memory (thanks) about the way I can get a default value from CommandBar.textPrompt().

But it looks like I need to repeat the various other comments I made here yesterday that I don't think you've picked up on:

pickDate() is currently seems to be inconsistent with how prompt() is called: <%- pickDate({'question':'Date of session'}) %> versus <%- prompt('place','Which school/group?') %> Can we bring pickDate() into line?

I'm also happy to rename these older functions as promptDate() and promptDateInterval() for consistency.

Looking at the only mention in the Templating docs, I see it's part of the page we improved but didn't finish. In particular I posed the question whether pickDate() and pickDateInterval() functions should become part of the system module, or a new helpers module.

Thoughts?

mikeerickson commented 2 years ago

@jgclark Ya, I did notice that, just let me know what you would like to do with the pickDate command. I can call datePicker method from @helpers/userInput.js if you would like.

I can change pickDate to call the revised datePicker method and use the same signature as prompt but that will have some potential migration issues?

Or, pickDate could be refactored to support both an object (which contains the question key) or support the new prompt formate

Eg

<%- pickDate('Enter Date:',...)%>

and

<%- pickDate({question: 'Enter Date',...}) %>

I am just concerned about all the templates which exist in the wild that will the the second syntax

mikeerickson commented 2 years ago

And, I can adjust documentation accordingly, just need to know what direction you would like to go with this

jgclark commented 2 years ago

Thanks, Mike. OK, how about the following:

jgclark commented 2 years ago

BTW, the change of signature in the new ones is important, as I realised that the existing pair don't land up with a variable that I can re-use later. That's a problem in my main use case for these functions, which is illustrated with this template:

---
title: Service Template
type: empty-note
---
### <%- prompt('place','Which Church?',['CCC','LSURC','Trinity Methodist','Other']) %> <%- prompt('sessionType','What sort of session is this?',['HC','MW','MP','EW','Special']) %>, "<%- prompt('sessionTitle','Service topic') %>", <%- pickDate({'question':'Date of service'}) %>
<%- prompt('notes','Passages and other notes') %>
* Music: liaise {-18d}
* Prayers: liaise {-5d}
* write Sunday service plan {-4d} at 1-2pm
* decide *** '<%= sessionTitle %>' sermon point {-5d} at 10:00-12:00
* liaise with children's groups for *** '<%= sessionTitle %>' sermon {-3d} at 1-5pm

Where it says *** I want to be able to re-use the date (albeit in a shorter form through use of the existing date manipulation functions).

If this additional requirement can be achieved with your "refactored to support both an object (which contains the question key) or support the new prompt format" then that's great ... but I'm not sure I'd attempt it, which is why I've suggested adding a new pair.

mikeerickson commented 2 years ago

@jgclark

Hmmmm, ok so I think I know what you are asking.

<%- pickDate('dateVar','Enter Date:','Use YYYY-MM-DD format') %>

But here

decide *** '<%= sessionTitle %>' sermon point {-5d} at 10:00-12:00

You want to use the value from dateVar and perform appropriate date math, in this case, subtract 5 days

* Something like <%- date.subtract(5,`${dateVar}`) %>

So, if you entered 2022-04-25, the value would appear as

* 2022-04-20

Is this what you are saying here? I think it already handles this using normal JavaScript interpolation, in this case access the dateVar variable defined in the pickDate item, and then using that later in the template by referencing that variable using interpolation.

I will test this to make sure it works, but that should already be in place


And

"If this additional requirement can be achieved with your "refactored to support both an object (which contains the question key) or support the new prompt format" then that's great ... but I'm not sure I'd attempt it, which is why I've suggested adding a new pair."

What do you mean by a "new pair"

I have made this adjustment, which accepts either an object or string and in the case of using a string, the actual prompt message is the second param, and also supports passing the default value as the 3rd parameters

#### Using Object Params
- <%- pickDate() %>
- <%- pickDate({question:'Please enter a date:'}) %>

#### Using String Params
- <%- pickDate('question1','Please Enter Date:') %>
- <%- pickDate('question2','Please Enter Date (format):','YYYY-MM-DD') %>
mikeerickson commented 2 years ago

@jgclark So, I am looking at your revised datePicker and I see you have changed the first parameter to be a string, but if a string is supplied, it then throws an error about invalid JSON in this line which fails because of the else part of the ternary

const paramConfig =
      dateParamsTrimmed.startsWith('{') && dateParamsTrimmed.endsWith('}')
        ? await parseJSON5(dateParams)
        : dateParamsTrimmed !== ''
          ? await parseJSON5(`{${dateParams}}`)
          : {} 

If string is passed (e.g. datePicker('Enter Date') then it is coerced to {${Enter Date}}

await parseJSON5(`{${dateParams}}`)

How have you tested this method when passing a string? Maybe I am not aligned with what you have done

I was thinking the calls to datePicker could be

datePicker('Enter Date')
datePicker({question: 'Enter Date'})

Would you please confirm desired implementation for datePicker method

jgclark commented 2 years ago

@jgclark

Hmmmm, ok so I think I know what you are asking.

<%- pickDate('dateVar','Enter Date:','Use YYYY-MM-DD format') %>

But here

decide *** '<%= sessionTitle %>' sermon point {-5d} at 10:00-12:00

You want to use the value from dateVar and perform appropriate date math, in this case, subtract 5 days ... Is this what you are saying here?

Sorry, no. I can already do the date arithmetic using my /processDates (though happy to have it possible through Templating too, as you go on to show).

I simply meant be able to re-use the value of the date variable like other prompt()-ed variables.

"If this additional requirement can be achieved with your "refactored to support both an object (which contains the question key) or support the new prompt format" then that's great ... but I'm not sure I'd attempt it, which is why I've suggested adding a new pair."

What do you mean by a "new pair"

See my previous message where I asked (for the third time) for a change of name of the functions to help make in line with prompt():

I have made this adjustment, which accepts either an object or string and in the case of using a string, the actual prompt message is the second param, and also supports passing the default value as the 3rd parameters


#### Using Object Params
- <%- pickDate() %>
- <%- pickDate({question:'Please enter a date:'}) %>

Yes, I'm suggesting that you leave this as is.

Using String Params

  • <%- pickDate('question1','Please Enter Date:') %>
  • <%- pickDate('question2','Please Enter Date (format):','YYYY-MM-DD') %>

Great ... if we change name to helpers.promptDate, and if 'questionN' is a variable name that can be re-used later.

mikeerickson commented 2 years ago

@jgclark ah, I see what you mean. The reason it Is currently not working because pickDate helper does not run through the prompt process as does using <%- prompt(…) %>

I will make it work the same and all should be good so that you can use the variable elsewhere in the template

<%- pickDate(‘test’, …) %>

<%- test %> // will be value from pickDate
webbj74 commented 2 years ago

I encountered the Invalid JSON5 error trying to use pickDate. I was also confused by the UI, as it did not seem any different than a regular prompt?

My expectations as a user:

mikeerickson commented 2 years ago

@webbj74 yes, a true date picker would be ideal and is something we have requested several times from Eduard, but at this time not available.

The next update from templating will have a new command promptDate (consistent with other prompt command) that will be in the form of your last example.

jgclark commented 2 years ago

@mikeerickson, thanks for sorting this out in Templating v1.1.0.

LaurenaRehbein commented 2 years ago

Hi @jgclark - the template you have on the ReadMe page for the Reviews plugin is returning this error:

The 'pickDate' helper has been deprecated, you should modify template to use 'promptDate(...) method

I am thinking this has something to do with this issue. Can you please confirm if the sample template should read something other than:

---
title: Create a new Project note
type: template, quick-note, empty-note
newNoteTitle: <%- prompt('noteTitle', 'Project name') %>
folder: '/'
---
#project @start(<%- pickDate({'question':'Enter start date'}) %>) @due(<%- pickDate({'question':'Enter due date'}) %>) @review(<%- pickDateInterval({'question':'Enter review interval'}) %>)
Aim: <%- prompt('aim') %>
Context: <%- prompt('context') %>

Thank you!

LaurenaRehbein commented 2 years ago

Solution:

#project @start(<%- system.promptDate('question','Enter start date:') %>) @end(<%- system.promptDate('question2','Enter end date:') %>) @review(<%- system.promptDateInterval('question3','Enter review interval:') %>)
jgclark commented 2 years ago

Thanks, @LaurenaRehbein, for reporting this out of date code example. It's now fixed.

dwertheimer commented 2 years ago

I have to run, but I can get you started:

I know what the JSON5 error is: the call signature is wrong. For the way the code is written today , the tag usage should be (note the curly brackets):

<%= pickDate( {question:'Date of session'} ) %>

…of course, if you want it to align with prompt as you said, then that code would need to be refactored.

There is still the [object object] problem to solve.

On Fri, Apr 22, 2022 at 5:29 AM, Jonathan Clark < @.*** > wrote:

Here's the start of the template definition:


title: School Session Template type: empty-note

<%-

prompt('place','Which school/group?',['GBP','FLJ','FLI','Cubs']) %> <%- prompt('sessionType','What sort of session is this?',['assembly','service','lesson']) %>, <%= pickDate('question','Date of session') %>

  • decide talk point {-7d} 10:00-12:00 ...

This runs OK, except that:

  • Before the date picker appears, it pops up this error message 'Invalid JSON5 in your configuration ...' -- i.e. there's some dependency still here with old nmn.Templates
  • It doesn't embed the given text in the note but instead writes '[object Promise]'
  • I think when I tried it with the pickDate as the first tag it asked the question last ... so possibly something odd with an await?

I've had a look but its not something I can quickly diagnose.

However, before diving in, let's just see work out what's best to do with pickDate, as I think it could do with some work to bring it in line with the rest of Templating.

  • Should it be renamed promptDate() ?
  • It's currently picking up code from helpers/userInput(), but would it be better to separate that off, and have this as a templates-only thing?
  • I hope in time that Eduard gives us a 'native' date picker UI element, which could then be used instead in the implementation of this.

Thanks.

— Reply to this email directly, view it on GitHub ( https://github.com/NotePlan/plugins/issues/269 ) , or unsubscribe ( https://github.com/notifications/unsubscribe-auth/ACEI6VD7HCULZJFCBOA66ELVGKLUDANCNFSM5UCF376Q ). You are receiving this because you are subscribed to this thread. Message ID: <NotePlan/plugins/issues/269 @ github. com>