| [19:47:23] | <symbioquine[m]> | > <@mstenta:matrix.org> > I think it is still valuable to set the time in many scenarios even when the user mostly "doesn't care" about it. |
| [19:47:24] | <symbioquine[m]> | > |
| [19:47:24] | <symbioquine[m]> | > Existing behavior doesn't change (if i'm understanding your thoughts), the time field just gets hidden with a link to show it. The default value of the time is the same with all these changes. |
| [19:47:24] | <symbioquine[m]> | Hmmm, I guess I'm probably misunderstanding https://github.com/farmOS/farmOS/pull/635 then... |
| [19:49:44] | <symbioquine[m]> | I was seeing diff lines like;... (full message at <https://libera.ems.host/_matrix/media/v3/download/libera.chat/60990de117...) |
| [19:50:21] | <symbioquine[m]> | * > Existing behavior doesn't change (if i'm understanding your thoughts), the time field just gets hidden with a link to show it. The default value of the time is the same with all these changes. |
| [19:50:21] | <symbioquine[m]> | Hmmm, I guess I'm probably misunderstanding https://github.com/farmOS/farmOS/pull/635 then... |
| [19:55:16] | <symbioquine[m]> | Okay, maybe I'm seeing what's going on better now... https://github.com/farmOS/farmOS/pull/635 probably does have that effect, but only in some specific cases like the planting quick form and the bulk "move"/"group" assets actions. Those places are only a subset of the forms that https://github.com/farmOS/farmOS/pull/637 would affect - most of the later retaining their existing behaviour of using the current time. |
| [19:55:19] | <symbioquine[m]> | Does that sound right? |
| [20:00:19] | <mstenta[m]> | Yea that's correct... and date('Y-m-d', ...) is the same as "midnight" so those didn't change |
| [20:00:57] | <mstenta[m]> | Basically it was taking the current date (without time) and converting it to a timestamp |
| [20:01:17] | <symbioquine[m]> | I guess that makes sense |
| [20:01:37] | <mstenta[m]> | The change kinda just made it more explicit by actually saying "midnight" :-) |
| [20:02:05] | <mstenta[m]> | And yea all the other forms still work the same |
| [20:02:19] | <symbioquine[m]> | Cool |
| [11:54:14] | <paul121[m]> | It would be cool if the "Show time" toggle was enabled by saying `#date_time_element => 'optional'` |
| [11:56:49] | <paul121[m]> | I was also thinking we should consider modifying our datetime_timestamp_optional field widget so it exposed some configuration for these #date_time_element options. The default datetime_timestamp widget we extend is kinda boring... :D |
| [11:57:50] | <paul121[m]> | It would be nice if you could specify any of the following 1) Require date & time 2) Require date, allow optional time and 3) Require date, do not allow time |
| [11:58:37] | <paul121[m]> | we would probably default to option 2) for most places in farmOS: require date, allow optional time |
| [11:59:30] | <paul121[m]> | but this might be a nice way to make that flexible |
| [12:03:02] | <symbioquine[m]> | ACTION uploaded an image: (205KiB) < https://libera.ems.host/_matrix/media/v3/download/matrix.org/KLcmCrVQLER... > |
| [12:10:59] | <mstenta[m]> | Oh neat ideas paul121 - I forgot about our `datetime_timestamp_optional` field widget 😄 |
| [12:11:44] | <mstenta[m]> | There is another distinction with all of this as well, perhaps: validation/constraints |
| [12:12:01] | <mstenta[m]> | The PR I opened is just a client side UI enhancement, doesn't enforce anything |
| [12:12:25] | <mstenta[m]> | > It would be nice if you could specify any of the following 1) Require date & time 2) Require date, allow optional time and 3) Require date, do not allow time |
| [12:12:25] | <mstenta[m]> | eg: these feel like they would imply validation/constraint logic perhaps |
| [12:14:40] | <paul121[m]> | Yeah that's true. But if you're going to require a date, or require a date and a time... the "show time" toggle doesn't seem necessary |
| [12:15:49] | <mstenta[m]> | agreed |
| [12:19:00] | <mstenta[m]> | > I forgot about our `datetime_timestamp_optional` field widget 😄 |
| [12:19:00] | <mstenta[m]> | Hmm so... is this related at all to https://github.com/farmOS/farmOS/issues/625? |
| [12:19:26] | <mstenta[m]> | Seems like the bug described there is exactly what datetime_timestamp_optional intended to solve...? Trying to dig through the history to remember... |
| [12:19:29] | <mstenta[m]> | Do you recall? |
| [12:20:05] | <paul121[m]> | Not quite. datetime_timestamp_optional fixed behavior where a timestamp would be saved, even if the user did not enter anything into the field widget. |
| [12:20:30] | <mstenta[m]> | Oh hmm |
| [12:20:44] | <paul121[m]> | eg: open animal asset form, do not touch the birth date field, but one would end up being saved |
| [12:20:56] | <paul121[m]> | * being saved (as current time) |
| [12:21:03] | <mstenta[m]> | ooooooh right i remember now |
| [12:21:16] | <paul121[m]> | really odd |
| [12:21:17] | <mstenta[m]> | thanks for the brain jog :-) |
| [12:22:31] | <mstenta[m]> | ah so... that makes me think... one (small) potential UX pitfall of "Show time" is: in optional date fields that don't get populated with a default value, if you just fill in a date and click save (without clicking "Show time" and entering a time) the validation would fail |
| [12:22:54] | <mstenta[m]> | unless we also added some logic to prefill time with 00:00:00 in those cases (somehow) |
| [12:23:16] | <mstenta[m]> | maybe some general logic to solve https://github.com/farmOS/farmOS/pull/636 would just be: if a date is saved without a time, set the time to 00:00:00? |
| [12:24:10] | <paul121[m]> | Yeah I think that is correct |
| [12:24:22] | <paul121[m]> | I think we're entering the realm of a custom form element haha |
| [12:24:39] | <symbioquine[m]> | paul121[m]: I think this is the correct behaviour - IMHO :) |
| [12:25:05] | <symbioquine[m]> | oh wait... |
| [12:25:11] | <symbioquine[m]> | Less for birth date |
| [12:25:27] | <symbioquine[m]> | Was thinking more like log creation time |
| [12:25:28] | <paul121[m]> | haha well for animal birth date that might be OK. But for other optional date/time fields it seems less ideal |
| [12:25:41] | <symbioquine[m]> | paul121[m]: Example? |
| [12:26:01] | <paul121[m]> | yeah it seems like the `datetime_timestamp` widget was kind designed for "entity created/changed time" ... |
| [12:26:35] | <paul121[m]> | I believe the lab test logs have two timestamps: received time & processed time (in addition to log timestamp) |
| [12:26:52] | <mstenta[m]> | so... could we just standardize everything on `datetime`? and add some submit logic to allow empty time, and add this "show time"? |
| [12:27:03] | <mstenta[m]> | (might not need a custom element?) |
| [12:27:21] | <mstenta[m]> | (i'm mixing a few things together here... trying to solve multiple problems perhaps) |
| [12:28:57] | <mstenta[m]> | We were using datetime_timestamp: https://github.com/farmOS/farmOS/commit/0416267cf9a6e6b6bb20e5d6fa102e10... |
| [12:28:59] | <symbioquine[m]> | mstenta[m]: But if the field isn't required, it should just be possible to leave it empty already right? |
| [12:29:00] | <mstenta[m]> | But maybe we could just use datetime? |
| [12:29:06] | <mstenta[m]> | symbioquine: yes agreed - if it's not required and it is empty, it should be possible to leave it blank |
| [12:29:11] | <mstenta[m]> | i think these two changes (back in pre-alpha i think?) fixed a bug we found with that... |
| [12:29:17] | <mstenta[m]> | https://github.com/farmOS/farmOS/commit/0416267cf9a6e6b6bb20e5d6fa102e10... |
| [12:29:22] | <mstenta[m]> | https://github.com/farmOS/farmOS/commit/79693d28dca32b625c460812a0603dc5... |
| [12:29:56] | <mstenta[m]> | I don't remember if there was a reason we used datetime_timestamp for that originally though (and not datetime) |
| [12:30:20] | <paul121[m]> | datetime = form element, datetime_timestamp = field widget |
| [12:30:29] | <mstenta[m]> | OHHH right. |
| [12:30:32] | <mstenta[m]> | thank you paul121 :-) |
| [12:32:46] | <paul121[m]> | I worry that modifying/altering the datetime form element may have unintended consequences. It's quite a low-level Drupal core thing, and other modules/fields/forms/etc may depend on its existing behavior (whether we think that behavior is right or wrong) |
| [12:32:46] | <mstenta[m]> | and we have two scenarios to solve for: Field API (higher level) and Form API (lower level) |
| [12:33:01] | <mstenta[m]> | yea that's the risk on my mind too |
| [12:36:52] | <mstenta[m]> | well, so just to bring it all back to specifics... it seems like this is the main issue with the current PR: |
| [12:36:52] | <mstenta[m]> | > one (small) potential UX pitfall of "Show time" is: in optional date fields that don't get populated with a default value, if you just fill in a date and click save (without clicking "Show time" and entering a time) the validation would fail |
| [12:36:52] | <mstenta[m]> | which *I think* (correct me if i'm wrong) is essentially the same "bug" (or just UX issue) reported here: https://github.com/farmOS/farmOS/issues/625 |
| [12:37:50] | <mstenta[m]> | they can't be solved the same way, necessarily, though... because of the two levels involved (Field API vs Form API) |
| [12:38:17] | <mstenta[m]> | because it requires validate/submit logic (I think) - which we COULD control in Field API, but not in Form API |
| [12:39:23] | <mstenta[m]> | (although maybe there's a way we could via a custom form element w/ validate method that populates time to 00:00:00 if one isn't set, but a date is) |
| [12:47:22] | <paul121[m]> | <mstenta[m]> "because it requires validate/..." <- I'm not sure, some of the validation might bubble up from form API! I wouldn't be surprised but haven't looked closely enough |
| [12:47:52] | <mstenta[m]> | > (although maybe there's a way we could via a custom form element w/ validate method that populates time to 00:00:00 if one isn't set, but a date is) |
| [12:47:52] | <mstenta[m]> | yea, that was where my mind went next with this^ |
| [12:47:59] | <mstenta[m]> | element-level validation |
| [16:56:28] | * YashGoel[m] has quit (*.net *.split) |
| [17:06:50] | * YashGoel[m] has joined #farmos |
