search

backdrop-contrib / date_all_day

GNU General Public License v2.0
2 stars 2 forks source link

Option to display date without "(All day)" appended #5

Closed olafgrabienski closed 7 years ago

olafgrabienski commented 7 years ago

Thanks for providing the Date All Day module, very useful! I'd like to use it however without the appended string (All day). It's often not necessary because in many contexts it is clear that a missing time display means All day. Additionally, on events where you collect an end date, the All day appendix looks quite repititive.

Could you add an option to not display the All day appendix in the settings? If that doesn't make sense in your opinion, I'd also be happy if the appendix was surrounded with a span tag making it possible to prevent its display via CSS.

Graham-72 commented 7 years ago

@olafgrabienski I am happy to look at how to do this.

I originally ported this because it would be needed by the calendar module (still incomplete). Have you found another use for it?

olafgrabienski commented 7 years ago

@Graham-72 Thanks for looking at my request! Yes, I found another use for the module: I have an event content type on my site. It's not displayed as a calendar but in a views list of upcoming events.

Graham-72 commented 7 years ago

@olafgrabienski I have started to look at this. I think that when only a single date is involved the addition of the suffix '(all day)' makes good sense. Where more than one day is involved I think we should perhaps have just one suffix after the two dates and it should be '(inclusive)'. What do you think of this idea?

And also we could have it enclosed with a span tag as you suggest.

olafgrabienski commented 7 years ago

@Graham-72 To display only one suffix when an an end date is involved, is a good idea!

In my opinion, there are also many cases where the suffix even doesn't make sense when only a single date is involved. Think for instance of 'events' like "Start of the summer holidays", "Full moon", somebody's birthday, or a deadline for a job application. Such events usually have no time specification, and an All day suffix sounds quite strange, or redundant in these cases.

It's hard to say if my examples are common enough to justify an additional option in the field settings. I'd say yes but if you disagree I'm also fine with controlling the display of the suffix via CSS.

Graham-72 commented 7 years ago

@olafgrabienski your examples are excellent. I agree that we need additional options in the field settings and I will work on these, one for the first (start) date and another for the second (end) date, for completeness.

Graham-72 commented 7 years ago

@olafgrabienski I am thinking that this would be a setting in the structure settings for the node type that is making an instance of the relevant date field, and not something that can be configured for each individual node i.e you can set it per node type but not per node. Would this meet your need?

olafgrabienski commented 7 years ago

Would this meet your need?

@Graham-72 Yes, absolutely! Sorry that I didn't clarify that before.

Graham-72 commented 7 years ago

@olafgrabienski I have now modified date_all_day.module in this repo. Please are you able to download and test it to see if it now does what we think it should? If all is well I shall then make a new release for the module.

olafgrabienski commented 7 years ago

@Graham-72 Thank you! I'll give it a try - at the latest on Monday.

olafgrabienski commented 7 years ago

Hey @Graham-72 Now I managed to test the modifications to date_all_day.module. Thanks again! The option to disable or modify suffixes added to the displayed date works fine: I'm able to define custom (or empty) suffixes to the start and/or the end date.

Here some improvement suggestions:

olafgrabienski commented 7 years ago

I forgot to mention one thing: After uploading the new module code with the new suffixes, they didn't appear until I saved the relevant field settings. Before, I got the following notices in DBlog:

Notice: Undefined index: all_day_suffix_1 in date_all_day_date_formatter_dates_alter() (line 61 of /(...)/modules/date_all_day/date_all_day.module).

Notice: Undefined index: all_day_suffix_2 in date_all_day_date_formatter_dates_alter() (line 62 of /(...)/modules/date_all_day/date_all_day.module).

Graham-72 commented 7 years ago

@olafgrabienski Thanks very much for your helpful comments and suggestions. I will be working on them.

Graham-72 commented 7 years ago

@olafgrabienski I have now pushed some changes to address the points you raise.

Make the default suffixes defined in the .module file (all day and inclusive at the moment) translateable.

Yes, though I have now changed them both to be 'all day' as their initial value.

Add a space character at the begin of the default suffixes....... Maybe it's better to put the suffixes in a span.

I have introduced a span and included the required space, also added a style class 'date-display-suffix' at line 142.

In the settings page of the date field, put the Suffix for start/end date settings directly under the Display all day checkbox option. At the moment, they are displayed below the Time increments settings.

There would seem to have been a clash with the field weight for the 'Time increments' in the Date module, so I have made changes here.

Always in the settings, remove "Default" from the labels.....

I agree with you that 'Default' is not a helpful word to use here.

Regarding the help text: instead of (...) suffix (...) for this content type, shouldn't it be (...) suffix (...) for this field?

The setting is actually just for the instance of the field i.e. its use in this content type. The same field can be used in a different content type where the suffixes may be different. This is in accordance with the note at the top of the settings form 'These settings apply only to the xxxx date field when used in the yyyy type.' Maybe there is a clearer way of expressing this.

Notice: Undefined index: all_day_suffix_1 ..... also all_day_suffix_2

Hopefully resolved now.

I am still doing some testing and I think there may be one or two changes to core's date module that would be helpful. Thank you for your very helpful feedback.

olafgrabienski commented 7 years ago

Great, I'll have a look at the changes tomorrow!

olafgrabienski commented 7 years ago

@Graham-72 I've had a chance to look at the changed .module file, it works fine now: Space and span for the suffixes: existing, position of the settings: perfect, help texts: good, notice about undefined index: gone!

The only issue so far: I can't translate the suffixes. I saw that you defined them in code but when I search for "all day" on admin/config/regional/translate/translate, the only result is "Display all day checkbox". Do you think that's because I changed the defaults temporarily? (I however switched back to the defaults before I searched for translateable strings.)

Graham-72 commented 7 years ago

@olafgrabienski I have just recently spotted and corrected my coding errors on lines 61 and 62. Has this perhaps been the problem?

olafgrabienski commented 7 years ago

@Graham-72 Tested it again with the corrected code which didn't change the issue initially. Then I created a new date field, so that it uses the defaults, as a result "all day" became translateable. So it seems, that the default is translateable, but a custom suffix is not. Unfortunately I've no idea how to make a custom string translateable.

Speaking of translations, I didn't test on a really localized site though, will do that later.

olafgrabienski commented 7 years ago

Just noticed another issue: The setting "Suffix for end date" is visible even if the option "Collect an end date" in the Field settings is disabled. It's not a real problem, I guess, but could confuse some users.

Graham-72 commented 7 years ago

the default is translateable, but a custom suffix is not

I am hoping that I have now fixed this.

Graham-72 commented 7 years ago

The setting "Suffix for end date" is visible even if the option "Collect an end date" in the Field settings is disabled.

I have now added an appropriate #states 'invisible' condition to the 'Suffix for end date' field.

olafgrabienski commented 7 years ago

the default is translateable, but a custom suffix is not

I am hoping that I have now fixed this.

Yes, seems to be fixed, great! Custom suffix string appeared on admin/config/regional/translate/translate after changing the suffix setting and saving some content with the relevant date field.

olafgrabienski commented 7 years ago

The setting "Suffix for end date" is visible even if the option "Collect an end date" in the Field settings is disabled.

I have now added an appropriate #states 'invisible' condition to the 'Suffix for end date' field.

Nice, that works as well!

Related idea: display "Suffix for start date" only if the "Display all day checkbox" option is enabled. (Does the following make sense? If a user disables the option after having set a custom start date suffix, the custom value should be saved anyway so that it can reappear when the user re-enables the all day checkbox.)

olafgrabienski commented 7 years ago

Noticed one strange translation loop issue:

Don't know how to handle such a case, also because I haven't tested the whole thing on a really localized site with reasonable language settings.

Graham-72 commented 7 years ago

Related idea: display "Suffix for start date" only if the "Display all day checkbox" option is enabled.

A good idea! Now implemented.

olafgrabienski commented 7 years ago

display "Suffix for start date" only if the "Display all day checkbox" option is enabled

A good idea! Now implemented.

@Graham-72 Thanks, tested the conditional display quickly, works fine!

olafgrabienski commented 7 years ago

@Graham-72 As far as I see, the issue is fixed now. Thanks for your great work!

Regarding the 'translation loop' question, I guess we don't have to handle it in this issue. I'll have a look at it at the next opportunity and will report in a new issue, if necessary.

olafgrabienski commented 7 years ago

If all is well I shall then make a new release for the module.

@Graham-72 Are you still planning to make a new release, now as the issue is fixed, or do you plan to wait, maybe because of other open issues?

Graham-72 commented 7 years ago

@olafgrabienski Yes, sorry for the delay. Thank you for all the help you have given. There is one issue that is concerning me regarding what happens when the end date is the same as the start date, but I think this is a problem in the core Date module. I hope to investigate this very soon but I have been a bit busy with other matters. I should be able to check this out and make the new release tomorrow.

olafgrabienski commented 7 years ago

@Graham-72 Thank you to have worked on it! And no need to hurry, I was just thinking about if I wait for the release for a particular site of if I patch the file in the meantime, but that can anyway wait for a couple of days.

Graham-72 commented 7 years ago

Now included in release 1.x-2.2.1.

olafgrabienski commented 7 years ago

@Graham-72 I've updated to the current release, looks good! I saw that you even added my name to the README, nice! (Btw, the links there to our Github profiles don't work, I guess because of the missing protocol in the URL.)

Another thing: I noticed that we now have a "Display all day checkbox" related condition regarding the "Suffix for start date" setting which not applies to the "Suffix for end date" setting. Didn't notice it before because I was still focused on the other end date condition (related to "Collect an end date").

klonos commented 7 years ago

Btw, the links there to our Github profiles don't work...

They do now 😉

olafgrabienski commented 7 years ago

@Graham-72 I have another follow-up to this issue. (Btw, do you prefer that I post such comments here, or should I open separate follow-up issues?)

The help text of the All day option on the node form says: If checked, times will not be needed and a suffix will be added to each date. The latter isn't necessarily true anymore, as a site builder can decide to not let display one of the suffixes or any suffix at all.

Proposal: If checked, times will not be needed.