Skip to content
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

Improve date readouts to use locale instead of fixed mdy/dmy behavior #626

Closed
gregallensworth opened this issue Oct 27, 2023 · 14 comments
Closed
Assignees
Labels
enhance New feature or request timeslider-v2

Comments

@gregallensworth
Copy link

gregallensworth commented Oct 27, 2023

Bug description

The Timeslider V2 can display dates as mm/dd/yyyy or dd/mm/yyyy

Which one it chooses depends on your chosen language's dateformat setting as discovered in L.Control.OHMTimeSlider.Translations This is either mdy or dmy

https://github.com/OpenHistoricalMap/leaflet-ohm-timeslider-v2/blob/dbbda56312a1b33b133d6e691c074b64cb4f8033/leaflet-ohm-timeslider.js#L975-L1109

Having it coded into this._translations is troublesome for two reasons:

  • It's not a translation string, and is the only of the translation elements to not be a string, so kind of an outlier
  • If your language isn't on the list, then it fails over to en-US's dateformat which is mdy So even if you have the UI translated through some other means, the dates are still coded this way

Could the UI detect your preferred date format ymd or mdy by testing a string in your locale, rarther than us hardcoding it for each language?

Repro Steps

  1. Go to https://staging.openhistoricalmap.org/
  2. Set your locale to one that isn't in timeslider e.g. Vietnamese, using Locale Switcher or similar
  3. See that the strings are not translated (that's expected)
  4. ... but also that the date formats are still m/d/y Again, not unexpected, but something we could improve even if we don't have language translations.

Improvement

Replace this._translations.dateformat with something better, still in discussion.

I see 13 places where this pre-determined dateformat is used:

https://github.com/search?q=repo%3AOpenHistoricalMap%2Fleaflet-ohm-timeslider-v2%20dateformat&type=code

  • dateformat defined in the 4 languages
  • dateformat mentioned in the README about adding new translations
  • 6 places where dateformat is used in a switch or if
  • 2 places where dateformat is mentioned in an error message
@gregallensworth gregallensworth added enhance New feature or request timeslider-v2 labels Oct 27, 2023
@gregallensworth gregallensworth self-assigned this Oct 27, 2023
@gregallensworth gregallensworth changed the title Improve date readouts to use Improve date readouts to use locale instead of fixed mdy/dmy behavior Oct 27, 2023
@1ec5
Copy link
Member

1ec5 commented Oct 27, 2023

For strings, we should be using Intl.DateTimeFormat. It takes a Date, but we have to be careful about how we construct that date: it’s important to use both Date.UTC() (to avoid errant time zone offsets) and Date.setUTCFullYear() (to correct two-digit year processing).

For arranging the dropdown menus, we can use Int.DateTimeFormat.formatToParts(), which gives us the date format in structured form.

@gregallensworth
Copy link
Author

I've been experimenting with Intl.DateTimeFormat per your advice, and I think that for the short formatting this could work well.

  • It does properly translate 0 to 1 BCE, -1 to 2 BCE, and so on
  • It does not add BCE if the year is <= 0 so we still have to do that ourselves
// short date
const y = 0;
const m = 5;
const d = 4;
const andthen = new Date(Date.UTC(y, m, d, 12, 0, 0, 0));
andthen.setFullYear(y)

console.log("BROWSER LANUAGES")
console.log(`${new Intl.DateTimeFormat(navigator.language, {dateStyle: 'short'}).format(andthen)}${y <= 0 ? ' BCE' : ''}`);
console.log(`${new Intl.DateTimeFormat(navigator.language, {dateStyle: 'long'}).format(andthen)}${y <= 0 ? ' BCE' : ''}`);

console.log("US")
console.log(`${new Intl.DateTimeFormat('en-US', {dateStyle: 'short'}).format(andthen)}${y <= 0 ? ' BCE' : ''}`);
console.log(`${new Intl.DateTimeFormat('en-US', {dateStyle: 'long'}).format(andthen)}${y <= 0 ? ' BCE' : ''}`);

console.log("UK")
console.log(`${new Intl.DateTimeFormat('en-GB', {dateStyle: 'short'}).format(andthen)}${y <= 0 ? ' BCE' : ''}`);
console.log(`${new Intl.DateTimeFormat('en-GB', {dateStyle: 'long'}).format(andthen)}${y <= 0 ? ' BCE' : ''}`);

console.log("VI")
console.log(`${new Intl.DateTimeFormat('vi', {dateStyle: 'short'}).format(andthen)}${y <= 0 ? ' BCE' : ''}`);
console.log(`${new Intl.DateTimeFormat('vi', {dateStyle: 'long'}).format(andthen)}${y <= 0 ? ' BCE' : ''}`);

console.log("ES")
console.log(`${new Intl.DateTimeFormat('es', {dateStyle: 'short'}).format(andthen)}${y <= 0 ? ' BCE' : ''}`);
console.log(`${new Intl.DateTimeFormat('es', {dateStyle: 'long'}).format(andthen)}${y <= 0 ? ' BCE' : ''}`);

console.log("FR")
console.log(`${new Intl.DateTimeFormat('fr', {dateStyle: 'short'}).format(andthen)}${y <= 0 ? ' BCE' : ''}`);
console.log(`${new Intl.DateTimeFormat('fr', {dateStyle: 'long'}).format(andthen)}${y <= 0 ? ' BCE' : ''}`);

The outputs are what we could hope for, so I agree, this could work well.

BROWSER LANUAGE (USA for me) 6/4/01 BCE June 4, 1 BCE
US 6/4/01 BCE June 4, 1 BCE
UK 04/06/1 BCE 4 June 1 BCE
VI 04/06/1 BCE 4 tháng 6, 1 BCE
ES 4/6/01 BCE 4 de junio de 1 BCE
FR 04/06/1 BCE4 juin 1 BCE

@1ec5
Copy link
Member

1ec5 commented Oct 30, 2023

${y <= 0 ? ' BCE' : ''}

Instead of hard-coding an era prefix, set era to short inside the options object. Also remember to set timeZone to UTC to eliminate the time zone offset.

@gregallensworth
Copy link
Author

gregallensworth commented Oct 30, 2023

1

The basic date readouts:

  • formatDateShort()
  • formatDateLong()
  • formatDateShortPlaceHolder()
    • generates placeholder values for display, either "mm/dd/yyyy" or "dd/mm/yyyy"
    • this is displayed to the right & left of the time slider so it's super-clear whether you're seeing d/m/y or m/d/y
    • however, it is those two fixed strings and should be translated; in French for example "annee" so should be "j/m/a"
    • new translation entry ymd_placeholder_short
    • and fill in for existing languages
    • remove formatDateShortPlaceHolder() function
    • and replace the 4 calls to it with calls to this.this._translations.ymd_placeholder_short instead

1B

Afterthought: For the BCE/CE pickers, the language can differ between our hardcoded per-language text in ymd_placeholder_short and JavaScript's Intl.DateTimeFormat output. Thus, the CE/BCE pickers in the range selector can show "aec" if we use my translations, or "a. C." in the readouts below.

While I was going for the more secular version of the BC/AD thing, but this is a UX inconsistency that's always visible. I wonder whether we can use Intl.DateTimeFormat here as well?

  • see if I can get just the era portion from Intl.DateTimeFormat reliably, without resorting to "split on space" shenanigans which would be unreliable
  • see if that era portion can be displayed for positive and negative so we have both strings
  • if so... then replace the 2 select elements' option labels with these
  • and remove the bce and ce translations that I used temproarily
q = new Intl.DateTimeFormat('es', {
  era: 'short',
}).formatToParts(date1);
era = q.filter(f => f.type == 'era')[0].value;
console.log( era );

2

At this point the this._translations.dateformat won't be used at all anymore so can be removed from translations.

  • remove dateformat translations

3

The Change Date modal comes with some other baggage that could be jettisoned along with changing it to a select-based UI or accepting only ISO. First cut, adjust this to allow only ISO dates and we can prune out more hand-crafted date string dependencies.

  • Change Date modal accept only ISO date
  • keyup event handler that enables the submit button
  • clean up unused functions that try to translate string dates to ISO

Then see #624 where I will replace the Change Date modal anyway. But this first pass will clean up a bunch of string-juggling stuff, reducing the code by a few paragraphs, and meaning less to adjust in that step.

4

The range UI is Month Day Year which is specific to USA and a few other place. Could those be rearranged depending on whether the given locale uses y/m/d or d/m/y ?

  • some function that creates some test date and then examines its string version, determine whether we're ymd or dmy, return that
  • startup layout, consult that function and change the order of...
    • the 3 bits of leaflet-ohm-timeslider-rangeinputs-mindate per that sequence
    • the 3 bits of leaflet-ohm-timeslider-rangeinputs-maxdate per that sequence

@gregallensworth
Copy link
Author

gregallensworth commented Nov 2, 2023

@1ec5 Check this out:

image

  • Preferred ymd or dmy is determined by a test date to Intl.DateTimeFormat, not hardcoded
  • Range selector, day and month swap with your locale
    • only mdy and dmy are supported; are there other locales where they want something like Y/d/m or something?
  • Current date readout is straight from Intl.DateTimeFormat so is formatted and translated by the browser
  • The "dd/mm/yyyy" placeholders at ends of the slider, are now programmed in L.Control.OHMTimeSlider.Translations so translate correctly, instead of being fixed to "mm/dd/yyyy" or "dd/mm/yyyy"
  • the BCE/CE selectors labels, are now from Intl.DateTimeFormat, not hardcoded

See instructions in OpenHistoricalMap/leaflet-ohm-timeslider-v2#11 if you want to test it out.

gregallensworth added a commit to OpenHistoricalMap/leaflet-ohm-timeslider-v2 that referenced this issue Nov 2, 2023
…ge picker, date outputs from Intl.DateTimeFormat
gregallensworth added a commit to OpenHistoricalMap/leaflet-ohm-timeslider-v2 that referenced this issue Nov 2, 2023
…n improvements and UI changes for ISO 8601 UX
@1ec5
Copy link
Member

1ec5 commented Nov 16, 2023

While I was going for the more secular version of the BC/AD thing, but this is a UX inconsistency that's always visible. I wonder whether we can use Intl.DateTimeFormat here as well?

Use formatToParts to get a structured representation of the formatted date, then replace the era with the preferred secular era (which we would need to supply from existing translations), then join the parts back together.

@gregallensworth
Copy link
Author

gregallensworth commented Mar 27, 2024

@1ec5 brought up the idea, that the months translation element, may not be necessary if we can get the names of all 12 months from Intl.DateTimeFormat.formatToParts()

months: ['Enero', 'Febrero', 'Marzo', 'Abril', 'Mayo', 'Junio', 'Julio', 'Agosto', 'Septiembre', 'Octubre', 'Noviembre', 'Diciembre'],

I should look that over once we get the other large diffs merged in from #624 and #634 and OpenHistoricalMap/leaflet-ohm-timeslider-v2#11

  • what uses the months translations array?
  • is it amenable to using a wrapper function instead, which generates them via Intl.DateTimeFormat.formatToParts() so we don't need to enter them manually?

@gregallensworth
Copy link
Author

In a similar note to the months question above, I wonder whether the datepicker_cebce string could also be removed?

If we give a positive date and a negative date, pass them to Intl.DateTimeFormat.formatToParts() and get the era bit, then we would get "BC" and "AD" or "a. C." and "d. C." or whatever, and have the two words.

If we could join this with something neutral such as a / instead of "or" then it wouldn't need translation.

  • what uses the datepicker_cebce string? would it look okay with / instead?
  • make up somethig to fetch the BC and AD strings, concatenate them
  • and use this wherever we use the datepicker_cebce string

@danrademacher danrademacher moved this from Todo - Known Path to In Progress in OpenHistoricalMap Dev Planning 2024 Mar 27, 2024
@danrademacher danrademacher moved this from In Progress to Todo - Known Path in OpenHistoricalMap Dev Planning 2024 May 14, 2024
@danrademacher
Copy link
Member

@gregallensworth is this ticket blocked by anything but your time/attention? I'm not sure if you need help figuring out next steps, or if this is something you could finish and close out.

@gregallensworth
Copy link
Author

@gregallensworth is this ticket blocked by anything but your time/attention? I'm not sure if you need help figuring out next steps, or if this is something you could finish and close out.

The pull request that changes the date formats to be locale-specific are in OpenHistoricalMap/leaflet-ohm-timeslider-v2#11 This same pull request addresses three items:

  • 626 = locale-specific date format (this one you're reading right now)
  • 630 = translations for French
  • 624 = new decimaldate which adheres more closely to ISO 8601

It is that final one (626) which is dicey. The server-side decimaldate written in Pl/PGSQL should also be updated at the same time as this frontend, or else the frontend and backend will be using different math for translating dates which would probably not do well. See #634 and https://github.com/OpenHistoricalMap/DateFunctions-plpgsql

@danrademacher
Copy link
Member

Ah, got it! I just spoke to Ruben about prioritizing that date functions PG update so we can merge and close out these blocked tickets.

@danrademacher
Copy link
Member

This PGSQL date function change has been merged and is now live in the data on staging.openhistoricalmap.org.

Gregor, if you could go ahead and merge the relevant PRs that depended on that, then we can deploy all to staging, make sure its working, then move to prod and close out!

@danrademacher danrademacher moved this from Todo - Known Path to In Progress in OpenHistoricalMap Dev Planning 2024 May 16, 2024
@gregallensworth
Copy link
Author

I have merged OpenHistoricalMap/leaflet-ohm-timeslider-v2#11

On https://staging.openhistoricalmap.org/ I now see the BC/AD selectors and date readouts adapting to a few randomly-selected locales, which I know we do not have specific programming here.

@danrademacher danrademacher moved this from In Progress to On Staging in OpenHistoricalMap Dev Planning 2024 May 23, 2024
@danrademacher
Copy link
Member

This is deployed and working

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhance New feature or request timeslider-v2
Projects
Development

No branches or pull requests

3 participants