There are complaints about the currently used time picker. A D7-issue already exists: #1676648: Alternative jquery timepicker for Office hours (D7)
However, in D8, a HTML5 DateTime field is in core. So, let's create this separate issue for D8.

CommentFileSizeAuthor
#18 2625714-18.patch63.06 KBdarrick
#13 office_hours_2625714-13.patch15.14 KBjohnv
#8 2625714-8.patch22.23 KBdarrick
#4 office_hours_2625714_4_time-element.patch6.81 KBjohnv
#3 office_hours_2625714_time-element.patch6.53 KBjohnv

Comments

johnv created an issue. See original summary.

johnv’s picture

johnv
Primary language Dutch
commented

darrick Posted an initial patch here: #2384453-10: Port office_hours to Drupal 8.
- It does not apply anymore to latest version.
- It does not show values after noon.

johnv’s picture

johnv
Primary language Dutch
commented
Issue summary: View changes
StatusFileSize
new office_hours_2625714_time-element.patch6.53 KB

Attached the updated path from #2

johnv’s picture

johnv
Primary language Dutch
commented
Status: Active » Needs review
StatusFileSize
new office_hours_2625714_4_time-element.patch6.81 KB

Another patch. I have been testing Datetime vs. Datelist.
@darrick, "Above you mention using the datelist element but I prefer the html time input field produced by the datetime element a bit better."

If you have limitation in the hours setting, then the list element needs more clicking, but gives 100% correct data. in the textfield widget, you get too many errors. Eg, '1820' is not excepted by the system, and '18:20' might not be a valid closing time for a location.
Perhaps we can switch between the two. If you apply this patch and chagne Datetime to Datelist, you get an interesting effect (flush cache first).

darrick’s picture

darrick commented

I did try using a datelist element. But the datelist element doesn't validate without the month, day and year parts. I tried adding those date parts as hidden fields but then ran into a huge learning curve. I'm really unclear about the order in which the valueCallback is called and how it sets the form_state. Especially when you have a element created out of elements. I.e. OfficeHoursSelect creates a Datetime or Datelist element. This is after changing the OfficeHoursSelect class to extend FormElement instead of Datetime or Datelist.

As for the textfield widget. I'm on chrome so the html5 time input is used. Which is nice. It's not supported on Firefox or IE. There is this issue about adding a fallback: https://www.drupal.org/node/1838234

  • johnv committed b8338d6 on 8.x-1.x authored by darrick 
    Issue #2625714 by darrick, johnv: use Datelist core element
johnv’s picture

johnv
Primary language Dutch
commented
Related issues: +#1838234: Add jQuery Timepicker for the Time element of the datetime field

See the patch. I added the datelist element for the reasons above. The office_hours_select element is now a wrapper around Datelist.
If we can find a way to convert the wrapper to a plugin, then we can swap the Datetime and Datelist elements via a field_setting in OfficeHoursList:

    $element['starthours'] = array(
      '#type' => 'office_hours_select', // datelist, datetime.
    );
    $element['endhours'] = array(
      '#type' => 'office_hours_select',
    );
darrick’s picture

darrick commented
StatusFileSize
new 2625714-8.patch22.23 KB

I did get something working well. Didn't get to making sure the various knobs are working (i.e. limitstart, limtiend, step, etc.)

I did figure out the issue with the valueCallback functions. In formBuilder::handleInputElement the form_state values are being built from the parent elements down to the children elements. So the parent element valueCallback functions are called first. If the valueCallback returns NULL (the default) the form_state value is set to the elements default_value. If the form_state value is already set the return from the valueCallback is ignored. The issue was the default_value to the parent was a array which set the parent element value but also in turn the values of the the child elements. So when the valueCallback for the Datetime or Datelist was called the value was ignored and their validate functions threw an error because the form_state values weren't correct.

To make everything work I had to create a OfficeHoursTable which extended the Table element to add a valueCallback.

Possibly to make it all cleaner the default_value to the OfficeTableList or OfficeTableSlot elements should be a OfficeTableItem class instead of an array.

The patch below is against your last commit for "Issue #2626910: Use DateHelper class"

The option to select either datetime or datelist is on the form_display screen. Default is datelist.

  • johnv committed fc81c20 on 8.x-1.x authored by darrick 
    Issue #2625714 by johnv, darrick: Add widget-setting list/textfield for...
johnv’s picture

johnv
Primary language Dutch
commented

I'm working with your patch, and try to understand. I've committed the part we agree ont: the widget settings and the new Table element.
Some remarks:
- your editor settings are not according to Drupal standard. Can you fix that?
- which Timezone do you want to use? Let's change that in another issue: #1925272: Timezone: Handle timezones We can use a placeholder for later recovery in each call. until then we can use "", NULL.

I'll repost the patch against latest HEAD.

  • johnv committed 6eded22 on 8.x-1.x 
    Issue #2625714 by johnv, darrick: Rebase code

  • johnv committed 4320fb0 on 8.x-1.x 
    Issue #2625714 by johnv, darrick: Rebase code
johnv’s picture

johnv
Primary language Dutch
commented
StatusFileSize
new office_hours_2625714-13.patch15.14 KB

Attached the version until now. It is a lot shorter, due to the previous commits. The extra error checks are now working. I am testing with the List widget, not the Week widget, as is is much cleaner. in that case. the Slot is not used.

johnv’s picture

johnv
Primary language Dutch
commented

BTW @darrick, do you know javascript? I need help with #2627934: Fix javascript in D8-version of office_hours module

johnv’s picture

johnv
Primary language Dutch
commented

It would be nice if the List/Slot could set the datetime/ datelist directly, and if we can strip the Select.

darrick’s picture

darrick commented

I did a bit more work on this. I think the OfficeHoursSelect element is a good idea and can be used elsewhere. To test, I created a widget for the datetime field type to allow a user to only select a hour. Worked well.

From there I rewrote the OfficeHoursListWidget to use the OfficeHoursSelect widget directly. With great success.

Then I made OfficeHoursDefaultWidget to extend the OfficeHoursListWidget but to handle multiple_values. I still had to use the OfficeHoursTable element.

I also modified the properties of the OfficeHoursItem to store the starthours and endhours as a datetime rather then int. This cut out a lot of code for converting starthours to a date value for use with the OfficeHoursSelect element.

Does this sound like a good direction to you?

If so, I'll submit a patch but need to double check all the field settings correctly. And the formatter needs some work. I'm thinking pieces of the formatter may be compatible with the datetime field too.

As for javascript I'm fair to middlin'. Thought for adding rows maybe cheat and wrap the rows in collapsible fieldset or the #states property. Copying rows probably easiest with javascript on client side. limitstart and limitend for ampm selection will also need to be done by the client.

johnv’s picture

johnv
Primary language Dutch
commented

This seems like great progress! I am curious about your patch.
All/most of the js code was working in D7. So there shouodnt be too much refubisment needed. I can look for the corrsponding open issue.

darrick’s picture

darrick commented
StatusFileSize
new 2625714-18.patch63.06 KB

Here's my current patch. Working well for the most part. Still haven't done anything with javascript and a few settings for the formatter aren't implemented.

johnv’s picture

johnv
Primary language Dutch
commented

Wow.

johnv’s picture

johnv
Primary language Dutch
commented

@darrick, how does this patch relate to #2384453-10: Port office_hours to Drupal 8 ? It has been a while, so I forgot.

johnv’s picture

johnv
Primary language Dutch
commented
Component: User interface » Code - widget
johnv’s picture

johnv
Primary language Dutch
commented
Status: Needs review » Closed (won't fix)

Let's forget about this.

The Drupal DateTime picker has 2 options:
- 'Select list' boxes for Y, M, D, H, M: We already have that.
- 'Date and Time' picker: there is no possibilty to control the granularity (enter data per 15, 30, 60 minutes)
Also, we will have problems, because OH does not contains dates.