Improve recurring theming: Show the end time and don't show the day during daily reoccurences

A simple solution to add end time that is working for our needs.

It maintains the existing "at ..." text if there is no end time, but otherwise uses the "date/time" join string from the format options.

Note that depending on the format's date/time separator an extra space could be introduced, like:

"Weekly on Monday and Friday , 10:00 a.m.–1:00 p.m."

... because it's being added for the predetermined " at" date/time join.

I am doing some post-processing to remove spaces before commas, but this should probably be fixed upstream in the solution here. Let's see if @mandclu accepts this idea in the first place and then proceed from there. :)

Thanks for the work to date on this. I added a couple of comments on the MR. If we can get those resolved this should be ready to merge.

I've provided feedback as to why certain choices were made.

@codebymikey I updated the MR by merging in recent commits in the target branch, and moving the capitalization of the repeat output to the twig template, which I believe is a better approach.

There's only one spot left to resolve, which is your proposed refactoring of the day output. Can you clarify what improvement this is intended to achieve?

@mandclu Appreciate the quick and thorough review!

Re: the latest comment, the improvement is tiny, but the main aim is to help with translations by putting the additional text inside a placeholder rather than concatenating the translated markup (just in case they need special handling in RTL languages).

References:

• https://www.drupal.org/node/322732 →
• https://www.drupal.org/docs/7/api/localization-api/localization-api-over... →

You should try to make strings for translation full sentences or phrases when possible, and never begin or end with a blank space. Instead of breaking up strings or embedding variables in the middle, use placeholders. Strings that may have several meanings in English can take a context attribute.

One other minor change to aid with the translations is to provide the Rule text context on all the $this->t() calls since some are still missing it within the PR.

I was looking at the Localization API documentation earlier, as well as some information about how lists of things work across languages. I agree with you that the way it assembles the string now isn't great.

I think the ideal state may be more complicated than it seems at first glance. Even in English, some prefer the Oxford comma and some do not. In French, the Oxford comma is never used, and from what I can see there are languages where they use other characters besides commas. So ideally individual sites could customize the output structure (e.g. to include the Oxford comma or not) but also be able to translate them.

I'm considering adding a separate theme template for the list of days, with translation tags included. It does feel a little heavy handed given the amount of information involved, but I can't think of a better way to handle this.

A couple of other things to note: I don't know if the changes in the MR address the extra space mentioned in #3 (also raised in 🐛 Can't get rid of space between Date and Date/time join character Active ) but if we could fix that as part of this set of work, it would be a good fit. Finally, I have some code I'm ready to test for ✨ Allow recurring formatter to express days of the week as ranges Active but I want to get these changes merged first.

The extra space issue mentioned in #3 should've been addressed as part of this MR, there's relevant logic within the default template preprocess hook - it's mostly redundant since the same spacing logic already exists in the main theme caller, it's just there as a reference for people who need to manipulate the spacing in a different manner.

I just added a commit that formats the comma-separated list of days in a twig template, which should allow sites to customize the output to meet their needs, across languages.

To address the space before the comma I ended up having to trim that output. For some reason, doing that caused the twig debug output to be escaped and therefore visible, so I ended up stripping tags. I don't love that, but since this was a literal string before anyway, I can live with it.

The capitalize twig filter didn't end up working, so went back to capitalizing the "every" string as originally suggested. There are also a few additional changes, but decided to go ahead and get the changes merged in.

Automatically closed - issue fixed for 2 weeks with no activity.