Closed (fixed)
Project:
Smart Date
Version:
8.x-2.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
11 Jul 2020 at 17:24 UTC
Updated:
27 Jul 2020 at 17:14 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
mandclu commentedHere's an initial patch. Would appreciate any and all feedback.
Comment #3
erik.erskine commentedHi @mandclu, I've applied the patch and had a quick go at using it on a core daterange field. Thoughts:
1. "All day" should go to
23:59:59rather than23:59:00as the core field type has second precision.2. The time field's visibility is based on the all day checkbox. Would the states API rather than smart_date.js be better for this?
3. I was confused by the presence of both "duration" and "end". Only when playing around with values did I see the change dynamically. But this is inconsistent with "all day" - why not just hide then end time completely if duration is anything other than custom?
4. I looked through smart_date.js and wondered about the resilience of this. In particular the reliance on class names. Would
data-attributes be better here? But would this logic not be better off done on the server, something like:if ($all_day == true) { $end = ... ; }Hope that's helpful!
Comment #4
mandclu commentedThanks for the feedback! My thoughts in response:
1. A valid point, but given the current state of the widget (still in Smart Date) will support both kinds of fields (and still uses a granularity by minute) I'm inclined to say that changing the end value created for All Day is better done at a different point, perhaps when this transitions to be part of Datetime Extras.
2. Good thought and I'm generally all for using functionality in core instead of something custom as much as possible. That said, the states API only works for entire fields, and in this case we're trying to show/hide part of a field. To use the states API we would have to build a new, fully custom field type, which seems like it would create a lot more custom code that we're using here.
3. I'm not sure I understand the point you're trying to make here. If you look at how popular calendar software works (Google, Microsoft, Apple, etc) you will see that they show start, end, and duration, and that they dynamically update based on whichever the user chooses to update. I would argue that from a UX standpoint we gain much more by leveraging what an editor is already familiar with.
4. I have tried to use front end JS to dynamically enhance the editor experience, but pair this with server-side validation to enhance constraints. Technically the "all day" element isn't a separate field so it's harder to validate on the server. If you have more specific ideas about where the code could be more efficient, happy to hear them! : )
Comment #5
erik.erskine commentedHmm, not sure it needs to be that complicated. The
DateRangeDurationWidgetin datetime_extras is already doing something similar with states.I'll try to explain what I meant with a screenshot from a calendar app. To add an event, you're first asked for one date/time, plus given options for duration (30 min, 1 hour, 2 hours etc). Only if you click "or choose an end date" does the second set of date elements appear. This text is equivalent to "custom" in your duration list.
I just found it a bit confusing seeing fields for the end date immediately followed by a separate duration dropdown. I saw start date, then end date, then duration - which seemed to be asking the same question. Only once I put in some values did I realise that "duration" was a kind of shortcut to doing that.
My question was really if the duration is something concrete, like "1 hour", is it necessary to show the end time at all? In the same way that "00:00" and "23:59" aren't shown when "all day" is checked.
Comment #6
mandclu commentedThanks for the explanation, I see what you mean now. In Google and Apple calendar, the duration works more like an autocomplete. I've opened #3158665: Implement Duration as autocomplete for end time as an issue for that specifically, and #3158666: Use core states API to show/hide times for using the States API.
Comment #8
mandclu commentedIt seems like the feedback to date (which is great) is really about how Smart Date works in general, and not specifically about how Smart Date works with core fields. Decided to merge this into a new 3.0.x branch, to make this easier for people to test and provide feedback. Feel free to reopen if additional issues are found.