-
-
Notifications
You must be signed in to change notification settings - Fork 825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make sure labels match the actual date input format #15520
Conversation
(Standard links)
|
@davialexandre Do you have an example of where I can see this in the UI / screenshot etc? |
@mattwire I've updated the PR description with the URL of the page where the field can be seen and some before/after screenshots |
@davialexandre yyyy was showing 4 four-digit year, e.g. 2009. Now PR has changed that to yy but the year is still 2009 and not 09 which is confusing. |
@yashodha I think you just made the confusion that I'm trying to avoid with this PR!
Besides using the date format configuration page, it's also possible to set that via the API. So what can happen is that people will go to the configuration form to see what date formats they can use and will see Personally, I think that the really confusing bit here is that |
@davialexandre If that's the goal then should the format string be outside the ts()? In some languages it translates to different letters even, which then wouldn't match the api. But then I wonder if it's english would they think it's wrong? Should it say somewhere on the screen or in the dropdown "api format"? |
@demeritcowboy I don't think it should be outside the The API shouldn't be mentioned anywhere in that screen as this is a global configuration for date formats in Civi. I only mentioned the API as another way to set the date format in Civi and where people can be confused. Independent of how you set it (using the API or the configuration form) the date format will be saved in the CiviCRM settings and will be used globally |
Taking a step back, why does the format string even appear on this screen if it's for regular users. They don't care about the difference between lower and capital M and date format strings. Maybe it just shouldn't be there at all and instead devs can right click and inspect to see format strings? |
@demeritcowboy this screen is more for admin users, so I think that it's not such a big deal to show the date format there. I would imagine this is something they know. What I think it doesn't make sense is to show the formats in a dropdown. The whole idea of being able to specify date formats is to give people more flexibility. If you limit that with a list of values in a dropdown, you're basically reducing this flexibility. This is outside of the scope of this PR though. |
@davialexandre @demeritcowboy I'm going to merge this - there are further improvements that could be made but this is a step in the right direction and fixes something in the UI that is wrong. |
Overview
The labels of the date input format dropdown (under /civicrm/admin/setting/date) don't match the actual value of the formats.
This might confuse people trying to set format using, for example, the API, as they might think that
yyyy
is the right way to display the full year.Before
The labels of the date input format dropdown don't match the actual value of the formats. Specifically, the labels shows
yyyy
as the format used to display the full year:Here's the markup for the field, showing the difference between the option values and their labels:
After
The labels match the actual formats and display
yy
as the format used to display the full year.Here is how it looks like after the update:
And here is the markup after the update, showing that only the options labels have been updated: