Problem/Motivation

Related to #2161337: Add a Date Range field type with support for end date

People want to have datetimes with start and end dates.

Proposed resolution

Add DateTime Range as an experimental module https://www.drupal.org/core/experimental

Proposal roadmap

Required sign-off before commit

Owner Status Description
Product manager
@Dries or @webchick
Pending
Framework manager
@alexpott, @effulgentsia, and/or @catch
Pending
Release manager
@catch and/or @xjm
Initial signoff on the approach. Pending followups for any issues raised after the experimental module patch.
Sub-system maintainers
Datetime module maintainer(s): @jhedstrom and/or @mpdonadio
  • @jhedstrom signed off on this being stable 2017-05-17
  • @mpdoandio signed off on this being stable 2017-06-03
Usability topic maintainers
(@yoroy and/or @Bojhan)
Pending
Accessibility topic maintainers
(@mgifford, @andrewmacpherson, and/or @jessebeach)
Pending

For this module to become a stable part of Drupal core, all of the "Must-have" and most "Should-have" followups should be completed before the beta phase for 8.4.0.

Must-have

https://www.drupal.org/core/experimental#requirements

Experimental modules are not subject to the same rigorous quality standards as stable Drupal core modules and do not yet need to meet all the Drupal core gates. They can be added with known architectural issues, imperfect coding standards (so long as they do not fail automated checks), minimal UIs, outstanding issues, etc.

These must-haves will need to be completed prior to 8.4.0-beta1.

Should-have

Could-have

  • Daterange specific argument, filter, and sort Views plugins: #2786577: Improve the Views integration for DateRange fields
  • This trait is specific to formatters, so perhaps rename to DateTimeRangeFormatterTrait? And maybe also move into the Drupal\datetime_range\Plugin\Field\FieldFormatter namespace?
  • Finally, if we're rerolling this patch anyway with any changes to the trait for the above, should we also move defaultSettings(), settingsForm(), and settingsSummary() to the trait, since those are identical between the 3 formatters? Even viewElements() could almost be moved there, so long as DateRangeDefaultFormatter could extend the trait's method and additionally handle the $item->_attributes logic.
  • Allow templating of the whole formatter to make date ranges easy to customize (beyond just the configurable separator), #2876434: Datetime Range fields should provide a template
  • Or, if templating is not provided, consider moving the space into the separator:
    +++ b/core/modules/datetime_range/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php
    @@ -0,0 +1,96 @@
    +            'separator' => ['#plain_text' => ' ' . $separator . ' '],
    
    +++ b/core/modules/datetime_range/src/Plugin/Field/FieldFormatter/DateRangeDefaultFormatter.php
    @@ -0,0 +1,104 @@
    +      

    This hardcodes the spaces surrounding the separator, should we move those into the separator variable instead?

  • Should the separator support HTML entities? (But: still ensure it's sanitized.)
  • https://www.drupal.org/node/2863439

Already complete in the experimental prototype (or not applicable)

  • check for security issues.
  • check for data integrity issues.
  • verify no disruption or regression to stable functionality
  • verify functional test coverage exists for the primary usecase, with no test failures in HEAD
  • document Ideas/prototypes for user interfaces that are planned
  • minimally validate prototypes for user interfaces
  • document any critical issues that maintainers identify as hard blockers
  • verify any critical issues that maintainers identify as hard blockers, are fixed
  • open followups for additional test coverage that might be needed in the future
  • verify basic API documentation exists for the minimum requirements
  • document a timeframe for completing the remaining work (typically two minor releases), a plan for addressing followups, and roughly which followups must be completed to keep the module in core.
  • Verify UI, API, Data module changes in this issue summary, they were copied from #2161337: Add a Date Range field type with support for end date
  • document a plan/open followups for completing more comprehensive documentation in the codebase and handbook (what topics are needed)

Final Steps

Review notes

When reviewing proposed experimental modules, consider whether your feedback is part of these minimal requirements, or whether it should instead be followup work as part of making the experimental module stable. If it does not need to block commit, post a followup issue on the roadmap instead of commenting on the initial patch.

[add more notes that are helpful for reviewers]

Contributed projects

https://www.drupal.org/project/datetime_extras

User interface changes

New field widgets.

API changes

New field type and associated goodness.

Data model changes

New schema for date ranges.

Comments

YesCT created an issue. See original summary.

YesCT’s picture

Was going to make this plan the parent of #2161337: Add a Date Range field type with support for end date but it already has a parent.

mpdonadio’s picture

Copying a comment from https://www.drupal.org/node/2161337?page=1#comment-11537963 so it doesn't get lost:

Before we do any real heavy lifting on this, can we pause for a moment?

The current plan is to add this as an experimental to 8.3.x and then make a decision about whether we can get it into 8.2.x? And it may not make it into 8.2.x anyway? And this is because of a bug that is in 8.2.x that just happens to be more evident in this patch, and we are currently spinning in circles on that issue? That issue also skirts the true problem, which I am going to post an issue about tonight.

If that is the plan, then I am not for it.

I would like one of two things to happen.

- The experimental gets into 8.2.x first
- or we commit 294 to 8.3.x, start fixing things as followups, and move on with everything in one module. People can use the patch against 8.2.x if they want. I don't see the data model changing at this point.

I thought the idea of an experimental module was so that we could get this into 8.2.x, so people could use it while we iron out the widget problem. I don't see what an experimental in 8.3.x buys us in the long run, especially since we are at the beginning of that development cycle. Long term, we either end up with two date-related modules, or having to do a nasty update path to get everything back into one module.

I'm still not sure why we went from several RTBC patches to an experimental module at the beginning of a development cycle. I am also not sure why we are jumping into an experimental module w/o some further conversations with the datetime maintainers.

YesCT’s picture

Issue summary: View changes

adding sign-off table to summary.

is a module maintainer a subsystem maintainer?

For reference
http://cgit.drupalcode.org/drupal/tree/core/MAINTAINERS.txt
http://drupal.org/project/governance

YesCT’s picture

Issue summary: View changes

added must/should/nice/could/not-in-scope headings

YesCT’s picture

Issue summary: View changes

html and some more headings

webchick’s picture

Yes, a module maintainer is a subsystem maintainer. I'm totally fine discussing this further as a course of action. It's certainly not anyone's intention to make this more difficult, esp. for site builders.

The reason it was suggested though is because it's clear many people are advocating for this solution to be in core. However, one 8.2.x deadline for the child issue (beta1 freeze - August 1) came and went. Another 8.2.x deadline (beta2 freeze - August 15) also came and went. beta2 was held for an additional 72 hours in the hopes that outstanding feedback could be addressed (August 19). That deadline also came and went, and now beta2 was already released over the weekend (August 20). Next step for 8.2.x is RC. The functionality at this point simply can't go into 8.2.x first.

The "patch" version of the issue is currently blocked on fixing pre-existing accessibility issues and display issues which this patch makes worse. This is because the patch is against a stable feature in core. Those issues look like they're thorny to solve, and might potentially take at least weeks.

So, in order to work around that problem, an experimental module was suggested. This eliminates the requirement for fixing the technical debt up-front, because then this becomes new, optional code that is therefore subject to much less stringent review criteria. Because this would be a new feature, it would go into 8.3.x.

In my mind, moving this to experimental code was going to be quick and easy, and only take a day or two. Then the code would at least be in a branch of core, and at that point we could discuss options for 8.2.x. Whether that's a backport of the 8.3.x module (which to be clear, I'm not sure we can even do anymore given we're actively trying to wrap up 8.2.x now) or a temporary contrib workaround (much easier when the code is segregated off into a module which can simply be listed as a dependency vs. a core patch which requires hacking core) or whatever.

But if the experimental module approach is going to create problems for maintainers and/or site builders, then the most sensible thing to do at this stage is to move the child issue to "postponed" on those accessibility/display issues, then get the patch into 8.3.x once those are cleaned up (plus whatever else was deemed commit-blocking, but those are the ones that stood out). That means no end date functionality for 8.2.x. I'm not trying to overrule subsystem maintainers, I was simply trying to reach a compromise that avoided forcing all site builders to wait for 8.3.x for this functionality, while respecting the release manager's decision.

Thoughts?

mpdonadio’s picture

Thanks @webchick.

I'm not going to speak for @jhedstrom (I am speaking as a datetime subsystem maintainer, though), but since getting this into 8.2.0 first is a non-starter (and probably 8.2.x at all), I think that adding this as an experimental module to 8.3.x is introducing technical debt to the project as a whole. It may not surface now, but it will eventually catch up to us. To an extent, I also think it defeats the purpose of the big refactor that happened between #236 and #256.

My suggested course of action is

- Get #348 into https://www.drupal.org/project/datetime_extras
- Address the root cause of the problem with the datetime widgets: #2788359: datetime_wrapper improperly applied to DateTimeWidgetBase ( #1918994: Improve Datetime and Daterange Widget accessibility hints at this, but I think misses a key point and it is current in bikeshed mode).
- Touch up #294 with the nits that got identified after
- Fix the daterange widget based on the patch I mention above. At this point we are now unblocked, and can commit to 8.3.x
- Get a migration / update path into https://www.drupal.org/project/datetime_extras so that when 8.3.0 come out, people can use the core version.

I think this will solve the issue with site builders needing this functionality and allow us to continue to work on datetime bugs and new features that hinge on the end-date patch (the queue doesn't explicitly show it, but there are a bunch of things soft-blocked on this). My only pause is the datetime_extras and core versions getting out of sync, either directly or because of bugs.

Thoughts?

[edited to include the issue I just created]

webchick’s picture

That all sounds fine to me. Tagging for release manager review.

xjm’s picture

Issue summary: View changes

This issue was not intended to add work. 90% of the items in the summary are complete. I will mark off those that are complete.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes

Stupid input filter is not applying the deleted tag to the whole list.

xjm’s picture

The difference between putting this in contrib and putting it in core is that in core we do not need an upgrade path.

The upgrade path from a contrib module to core is really, really complicated.

xjm’s picture

To be clear, the plan would be for datetime_range to become a stable module, alongside datetime itself. No additional refactors nor data model changes would be planned (and if we discover later that we would need them, that would happen regardless of where the code lives).

When I reviewed the experimental module, it was still currently extending the datetime classes, so at most I think there would be a couple 8.3.x issues to move the only deleted lines in the #294 and their related changes back into the datetime module, but that would be API additions or internal API changes, so very little disruption. And the scope of those things was very small when I reviewed @swentel's patch. Maybe I've missed something but I don't think the work is being undone by having it as experimental?

xjm’s picture

Ah, one more thing I think is getting forgotten:

I'm still not sure why we went from several RTBC patches to an experimental module at the beginning of a development cycle.

An RTBC patch is not a committed patch. While the patches were marked RTBC, they were not committable. The same bugs exist regardless. We can't just divert things to followups for stable functionality. We can for experimental modules, though.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes

Removing some unneeded sections from the summary.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
mpdonadio’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes

Adding a note about the trait changes (we can look into whether they make sense for the base field type as well). Also for the accessibility work that this field needs in addition to the existing ones (the direction @mpdonadio started on #2161337: Add a Date Range field type with support for end date looked like the correct markup for combined start and end dates; probably a separate issue once #1918994: Improve Datetime and Daterange Widget accessibility is fixed).

xjm’s picture

Issue summary: View changes

Adding link to the diff showing what refactor work we should consider in a followup.

jhedstrom’s picture

Thanks to everybody for all the hard work over the past several days.

I have some questions and concerns about moving this forward as a new experimental module that I'd like to voice here. None of them are blockers by any means, but I feel we need to pause and think about the ramifications of this approach.

  • What are the realistic prospects for porting this back to 8.2? I see no reason to go down the experimental module route if that is not a certainty.
  • AFAIK, core has no precedent for removing a module (in a non-major release), resulting in a big upgrade path. If removal is a possibility, we'd potentially need to mark every function and class as @internal to avoid API breakage.
  • If the intent is to forever maintain a separate module, once 'stable', are core committers committed to getting this new module into the standard install profile? If not, this will be a usability disaster as folks struggle to understand they need to enable another module to add an end date.
  • Once this is in an experimental module, it will receive far greater scrutiny than the current datetime module ever had, potentially making it a very long journey to lift the 'experimental' status. Contrast this with the known effort to make the parent issue acceptable to commit to the existent datetime module.
  • mpdonadio's plan in #8 makes sense to me if folks are still amenable to not doing this in a separate core module.
  • Given how quickly we went from an RTBC patch, to the introduction of a new core module, I think this potentially sets an unwanted precedent for future core work.
  • This is moving so fast (bear in mind the parent issue has been underway for 2 years), I personally haven't had a chance to think through the potential difficulty of fixing other date issues if this is in a separate module (for instance, we'd require separate Views plugins, whereas the approach in 7.x was able to leverage shared plugins for dates with and without end dates).
xjm’s picture

Issue summary: View changes

Thanks @jhedstrom. I'll try to answer some of your points with the most important one first:

If the intent is to forever maintain a separate module, once 'stable', are core committers committed to getting this new module into the standard install profile? If not, this will be a usability disaster as folks struggle to understand they need to enable another module to add an end date.

Yes, the end goal is making the module stable and a part of Standard based on the discussion @webchick and I had. I'll add this to the summary. Otherwise I think the experimental module approach would be a non-starter.

Given how quickly we went from an RTBC patch, to the introduction of a new core module, I think this potentially sets an unwanted precedent for future core work.

Again, keep in mind that the patch was marked RTBC, but at no point was it committable as a stable feature. Based on your remarks on the other issue, I'd actually encourage experimental modules for datetime functionality in the future as well. I think it's a safer approach (and would also have been less stressful if we had suggested it back around when the patch got changed to a new field rather than modifying the existing one). That's exactly the sort of problem-solving that experimental modules are a good fit for.

Once this is in an experimental module, it will receive far greater scrutiny than the current datetime module ever had, potentially making it a very long journey to lift the 'experimental' status. Contrast this with the known effort to make the parent issue acceptable to commit to the existent datetime module.

It's probable we did not do enough usability and accessibility review early on for datetime. And indeed, we already have one existing accessibility issue as technical debt that the experimental module needs to address. This is a good thing, though.

This is moving so fast (bear in mind the parent issue has been underway for 2 years), I personally haven't had a chance to think through the potential difficulty of fixing other date issues if this is in a separate module (for instance, we'd require separate Views plugins, whereas the approach in 7.x was able to leverage shared plugins for dates with and without end dates).

I'll bet you a DrupalCon sticker dawehner could come up with a suggestion for how to reuse the Views plugins, if we ask him. :)

And finally:

What are the realistic prospects for porting this back to 8.2? I see no reason to go down the experimental module route if that is not a certainty.

We cannot make any promises, because we need to release on schedule. I am considering backporting it (even though this would be against policy and mean I have to release a third beta), but it depends on the final patch and when it lands. It's not something open for discussion (so please don't anyone write me any poetry about date ranges), but it is an option. Only once it's committed, though. We are a little lucky in that there are five Wednesdays in August (which means there are still two weeks before the scheduled RC).

For background, given our experience with this issue, in the future we will probably not do the "beta target" exceptions at all. Issues will all be committed to the next branch, and a few might be backported at committer discretion if they are strategically important. But it's a decision made after commit rather than something affecting the issue while it's open. One of the difficulties we ran into was that "RTBC" is not necessarily "Done" (and "Done" also includes a committer having time to review), so we are going to shift the focus to issues that are actually done while we prepare the minor.

jhedstrom’s picture

Thanks @xjm for the very detailed follow-up. This addresses most of my concerns, and I am willing to support this approach.

Will this issue become the plan to remove experimental status, or will that be tracked as a separate issue?

webchick’s picture

I think this issue probably makes sense for that, since it has a list of outstanding @todos.

I'm fine with the plan to get this module stable and get it in standard profile, but I'm curious about this:

are core committers committed to getting this new module into the standard install profile? If not, this will be a usability disaster as folks struggle to understand they need to enable another module to add an end date.

Modules such as Event, Calendar, Sign-up, or whatever distributions there are that require end dates can simply add this module as a dependency, and it'll get automatically turned on when those modules get turned on. I'm not understanding the usability disaster concern.

xjm’s picture

Modules such as Event, Calendar, Sign-up, or whatever distributions there are that require end dates can simply add this module as a dependency, and it'll get automatically turned on when those modules get turned on. I'm not understanding the usability disaster concern.

I think it's more about the out-of-the-box core user experience, especially as more datetime features are added. If everything except end dates eventually ended up being supported by the datetime module, which shipped with standard, but only the one feature to allow end dates you had to know to find and enable a separate module for, that'd kinda be a WTF. (Not a disaster, but non-ideal.) It'd be like shipping Standard with all the current field types except Number enabled.

Contrib will need to declare their dependencies on the modules either way. In any case, though, I think we're agreed on targeting it for Standard once it's stable.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Collecting more unaddressed points of feedback from the issue. We're going to need someone to help out filing followups. There are numerous things among them that I would have blocked commit on (in addition to the stuff with the field labels), so I'm very glad we've agreed to go forward with the experimental module.

mpdonadio’s picture

#34, once we have all of our feedback collected, I will create the list of followups with @jhedstrom's help. The main reason is I want both of us to look at the existing datetime issues and coordinate them and the followups into a proposed dependency tree for three reasons.

One, if we want to do this right, we want to get some of the refactoring back in so the experimental uses more from the stable module to put us in a good position for the followups and planned additions to the stable module that would also go into this (eg, the timezone work @jhedstrom has been plugging away at).

Two, I want to avoid re-roll hell from when patches land, as much as possible, from when patches no longer apply. This may require some intermediate tasks, such as splitting up the field tests into separate tests (could also be part of BTB conversion).

Three, I would like each followup to have an owner who stays out of work so they can review, to avoid patches sitting as much as possible.

And thank you for the time you are putting into this.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Also rearranging the issue parenting so that this is the meta for date range and a child of the overall meta for the subsystem.

xjm’s picture

xjm’s picture

Issue summary: View changes

Regarding the RTL support, I think we actually might be victims of our own LTR assumptions in considering that point. A + B + C is logically the same in LTR or RTL; it's just rendered the other way around, by the browser. So that point might actually be a non-issue, but we should at least confirm it with testing. However, translatability in general for the formatter is still an issue.

jhedstrom’s picture

re #27 and #28, indeed, I was referring to the out-of-the-box install (which, afaik, is typically used in the usability testing?). The word 'disaster' might have been a bit strong, but yes, a Drupal WTF indeed in that case.

With regards to the contrib modules requiring the date range module, I'm not sure that makes sense either, since hypothetically one can make a calendar with only start dates. I'll leave that to the contrib maintainers though :)

xjm’s picture

In #2161337: Add a Date Range field type with support for end date apparently a couple contributors thought we had accidentally overlooked not adding a separate component for datetime_range, though actually I had made the decision on purpose given that really the module is part of the same subsystem conceptually and even has very interconnected bugs etc. I worry that having a separate subsystem further exacerbates some of the downsides to having the experimental module. @jhedstrom and @mpdonadio, can you confirm that you have no objection to me keeping it as a single component?

jhedstrom’s picture

@jhedstrom and @mpdonadio, can you confirm that you have no objection to me keeping it as a single component?

I would prefer a single component myself.

xjm’s picture

Thanks @jhedstrom -- I will re-merge the components.

mpdonadio’s picture

RTL testing:

English

Hebrew

So, I think this is OK. I do agree, though, that a separate template for this field would make sense.

mpdonadio’s picture

Issue summary: View changes
mpdonadio’s picture

Issue summary: View changes
mpdonadio’s picture

Issue summary: View changes
mpdonadio’s picture

Issue summary: View changes
mpdonadio’s picture

Issue summary: View changes
xjm’s picture

@catch, @cilefen, @Cottser, @alexpott, @effulgentsia and I discussed the status of this module's roadmap yesterday and agreed that this module is still in alpha since there are still a number of must- and should-have issues that need to be addressed. As a reminder, the module needs to become stable by 8.4.0-beta1 in six months to remain in core.

It's great to see the progress that's been made on DateTime range since it was added!

mpdonadio’s picture

Issue summary: View changes

RTL support is working properly, per Gábor's look. So, having a template for these is somewhat of a non-issue for the plan, but probably still a good idea for the long run.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mpdonadio’s picture

I just want to stress that we are all in this together if we want to keep Date Range in core.

There are two core gates remaining from the list: #2788359: datetime_wrapper improperly applied to DateTimeWidgetBase has been sitting at Needs Review for nearly three months. It is soft blocking #1918994: Improve Datetime and Daterange Widget accessibility, which is a core-gate according to the plan, and that issue hasn't been touched in a very long time.

The remaining Should Haves are mostly done, though more refactoring/cleanup is in the works, some of which is waiting on the TZ patches to get committed.

mpdonadio’s picture

Issue summary: View changes
xjm’s picture

Issue tags: +DrupalCampNJ2017

@mpdonadio and I discussed the blockers for getting to beta priority yesterday at DrupalCamp NJ.

mpdonadio’s picture

Issue summary: View changes

#2775669: Clean up redundant methods in datetime field formatters is in, which represents a good deal of refactoring, and should satisfy the "should have", but I think once #1918994: Improve Datetime and Daterange Widget accessibility lands, we can do one more polishing pass at the widget/formatter internals. See also #2793143: The datetime and daterange formatters (except plain) should use the HTML time element.

Stephen Ollman’s picture

Is Date Range (with Views integration) working out-of-the-box as at 8.3.0-rc1?

mpdonadio’s picture

#58, the field is in 8.2.X, but #2786577: Improve the Views integration for DateRange fields has not been committed yet and more than likely won't be in 8.3.X. There is basic views integration, but not at the same level as with normal datetime fields.

claudiu.cristea’s picture

@mpdonadio, excepting #2786577: Improve the Views integration for DateRange fields, what other critical functionality should we expect that will be present in 8.4.x but missed from 8.3.x? Asking because on our project we are on 8.3.x but we need Datetime range.

mpdonadio’s picture

#60, it's not a feature, but #2627512: Datetime Views plugins don't support timezones is looking 8.4 only, but the patch works with 8.3. I don't think we are looking at new features in 8.3, just cleanup and stability.

Different widgets/formatters will always be accepted into datetime_extras if someone want to work on them.

mpdonadio’s picture

@xjm, can we get a status update on this issue?

I think all of our must-haves and should-haves are done at this point, per the list in the IS. We could do a little more refactoring of the widgets and formatters, but that is really tied to bugfixes and slight changes to how we want these to work. In other words, I am happy with the cleanup that has happened and more will happen as we continue to mature these.

Right now, work is being focused on datetime and daterange as a unit (while keeping them uncoupled), so I think this is ready for signoffs.

xjm’s picture

The RTL in #45 looks wrong to me at a glance (I could be mistaken); the dates are ordered correctly but the whole field is aligned left?

Can we also file issues for the Views stuff and other could-haves? It is not required to fix them, but we should capture them in feature requests at a minimum.

Finally, I think it would be good for the usability team to do a final review of the finished module (this is a must-have requirement; the linked issue noted next to it was resolved but the usability review itself should happen).

Other than that it looks like the roadmap is mostly complete! Thank you @mpdonadio and @jhedstrom!

xjm’s picture

Issue tags: +Needs followup

Ah, this tag actually applies for the could-haves.

xjm’s picture

Oh also, all the stuff in #63 is a blocker for RC, not beta -- I think we can probably mark the module beta for 8.3.0 even! We can do that during the RC phase of core since the module's stability is still lower than core's. I'll get a second opinion on that.

xjm’s picture

Ah, #2775669: Clean up redundant methods in datetime field formatters was fixed in 8.4.x only since it was refactoring stable code as well, so we should consider whether the beta is for 8.3.x or 8.4.x based on that. It still might be fine for 8.3.0 since the forward change is BC (right?).

mpdonadio’s picture

#66, #2775669: Clean up redundant methods in datetime field formatters didn't go into 8.3 b/c the #plain_text vs #markup change and I think the general 8.3 timing (I think we just missed alpha1). Everything else should be BC with 8.3; no API differences, just shuffling some code around. I'm fine with beta for 8.4; not getting kicked out of Core is the only thing I really care about :)

RTL, Gábor had a POV on that in https://www.drupal.org/node/2787105#comment-11869670. Any alignment problems sound like a general problem with those wrappers, and not limited to daterange. I'll play with that later (not on a machine where I have an install where I can mess with languages right now). But, still playing dumb American card; I am not very knowledgable about this.

I will note, that the usability/accessibility team put the widgets through the wringer :) so I am not really worried about them.

I'll cleanup the IS and make a few followups tonight; we have had informal requests, but no issues yet.

yoroy’s picture

Tried the module, this is what I found:

In my first attempt I picked the existing date field because:
1. I expected date ranges to be a feature of date fields
2. The date range item is not alpha sorted to show up directly below date

Re 1: Could be something to strive for eventually but that's not a must or should for now
Re 2: This we should fix so that "date range" shows right below "date" in the field select list.

3. There's very explicit whitespace on the top part of the fieldset because there's a span tag inside the legend that's set to visually-hidden via CSS.

re 3: Is it hidden intentionally? Why? If by design, can we do it in a way that does not introduce the additional whitespace?

Configuring how to display the date (+ time) all seems to work great, defining a custom seperator is yummy. Not related to this issue but eww at how we present the values for date formats in the select list:

Anyway,

4. And then I had to test how it would work if I'd set the end date to before the start date :)

It's good to see that that scenario is accounted for but it would be much better if we could prevent people from doing this in the first place. Probably by making the date-picker restrict its available options to only dates after the start date.

---

Initial assessment:

- The experimental version needs work for 2 and maybe 3 if it is relatively simple to fix
- Looking ahead at stabilizing we'd want to work on 4. Item 1 is likely a much wider discussion. Though intesting, no worries for now.

mpdonadio’s picture

Issue tags:

@yoroy, thanks for the quick response to this. Do you think these are specific to the datetime_range.module, or general Drupal bugs?

With #2, nothing in that list is alpha-sorted, so I think this is a core-issue that is worse for Daterange.

With #3, the visually hidden things are the label elements for the individual form elements; they are a11y helpers and all of that is coming from core form elements. Since the module is reusing other parts of core, and not defining any of its own theming, I am struggling with whether we have a daterange bug or a core bug that manifests itself in a more ugly manner in this module? That has really been a big stuggle with with all the UX/A11Y issues we have faced.

yoroy’s picture

1. Not sure this needs a followup now, doing something about 2 below would be a solid first step in the right direction
2. Agreed it's a core issue to better sort the "General" section in the fields list.
3. Accessibility is hard :) I *think* the current output is correct, but it would be nice to style it better. Not daterange specific, so a followup for core in general.
4. Very much daterange specific and pretty major in UX terms. Needs a followup but not blocking imo.

I'll have a go at those followups.

yoroy’s picture

yoroy’s picture

yoroy’s picture

gambry’s picture

Issue summary: View changes
mpdonadio’s picture

Re RTL, here is a new screenshot that shows proper alignment. I wouldn't be surprised if the wrapper issue fixed this, too:

Still need to do an IS cleanup and make f/ups for

- formatter template
- entities as separator (this may work now since we are using #markup instead of #plain_text)

mpdonadio’s picture

Are there any updates on what the status of this is and whether we have any blocking issues to remain in core?

mpdonadio’s picture

Issue summary: View changes
mpdonadio’s picture

This issue has had Needs release manager review for a while now, but per a quick conversation with @xjm, I would like to request

- What is blocking the module being marked as beta?

- Other than the signoffs, what is blocking the module being marked as stable?

As far as I know, we don't have any blocking issues. We have followups, but they aren't on the critical path for the module's plan.

jhedstrom’s picture

Issue summary: View changes

I've updated the IS to indicate I am signed off on this being stable.

mpdonadio’s picture

Issue summary: View changes

Still not sure what the blocking issues are for this to become officially beta and/or stable. Adding my signoff that this is stable, too.

Bojhan’s picture

Can a framework manager review, and confirm that this indeed doesn't have blocking issues to move to officially beta/stable.

xjm’s picture

That's actually a release management decision. :)

FWIW, I think

xjm’s picture

...Clicked save accidentally in the middle of typing my comment. I think we need to add the issues @yoroy identified onto the roadmap. It sounded like #68.4 was considered very important? It looks like everything I identified originally is complete (yay!). Are there other issues that are not listed in the summary, other than the ones @yoroy identified?

xjm’s picture

I think #2863444: Discourage/make impossible to select a "to" date that is before the "from" date is the one; there we go. So that should be a "Should". And the others are "Could". Sound good?

(Rhymes!)

Barring any surprises, I also think we're ready to call this stable but would like to verify the list of any other issues that may exist first. Ideally we would push forward #2863444: Discourage/make impossible to select a "to" date that is before the "from" date as well since that is a major bug we'd be introducing.

xjm’s picture

Hm I don't think #2701867: field types in the select list on the 'Add field' page are incorrectly sorted was the correct issue so let's add an issue for "Make Date Range appear immediately under Date in the field type selection". We can do whatever evil necessary to accomplish that but it will prevent people from finding the thing they need if they're not right next to each other.

xjm’s picture

Oh and we need an issue for:

Followup step: Add the stable module to Standard.

If the UX issues in #84 and #85 are addressed in the next month, we could even add it to Standard for 8.4.0 (although we'll want the product team to give their +1 before we do that).

Sorry for posting like 8 comments in a row. :)

xjm’s picture

Issue summary: View changes
Issue tags: -Needs release manager review

Updating the IS based on my feedback (and also removing some outdated text so it's easier to read).

mpdonadio’s picture

xjm’s picture

let's add an issue for "Make Date Range appear immediately under Date in the field type selection".

I think we still need an issue for that, unless I missed it?

mpdonadio’s picture

Issue summary: View changes

Right now do we have anything blocking #2893128: Remove datetime_range from experimental package?

xjm’s picture

I looked over #2863444: Discourage/make impossible to select a "to" date that is before the "from" date and I can see how it is a whole can of worms, so I agree with @yoroy on keeping it as a major issue but not blocking the module's stability on it.

Let's look into #89 though, which is a relatively small change that will probably make a big difference for people being able to select date range fields successfully.

xjm’s picture

Issue tags: -Needs followup

Ahh, I see, #2701867: field types in the select list on the 'Add field' page are incorrectly sorted was in fact about the sorting of the field type list (for #89). I see that it was closed as cannot reproduce when I commented before, which is why I was confused.

Okay, I can't explain that either, but the issue does exist.

I'll reach out to the other committers to confirm that they're all agreed for the module to be stable!

Wim Leers’s picture

Congrats, mpdonadio!

🎉🎉🎉