624 ISO 8601 and BCE + 626 Localization improvements #11
Conversation
…ge picker, date outputs from Intl.DateTimeFormat
…n improvements and UI changes for ISO 8601 UX
|
I just tested this locally and it seems to work well just in terms of UI selectors. Is there any reason we need to coordinate this merge with the updating of PG functions in the tiler database (634)? I know they are topically related, but I think they can happen independently of each other, correct? |
|
Also, @1ec5 do you want to take a look and test this locally? |
There was a problem hiding this comment.
Thanks for your patience. I had been holding off thinking that it was still in progress to include more formatting changes. I played around with it, but admittedly I don’t feel very confident in my own ability to gauge the correctness of these computations. What would help is some sort of automated test that goes over the inputs and outputs we’ve agreed on. Tests are more important the more manual date math we end up doing.
My feedback centers around the idea that we should be doing as little manual date math as possible. The world already runs on the Date implementation in browsers, so we know it works correctly, modulo some well-documented quirks. We can land these improvements as a first step if you prefer, but I think the library would benefit from the simplification that migrating to Date would bring.
Since OpenHistoricalMap/issues#626 is referenced here, is the plan to migrate to DateTimeFormat in this PR or in a followup PR? This will all but require the migration to Date.
| decimaldate.proportionofdayspassed = (yearint, monthint, dayint) => { | ||
| // count the number of days to get through the prior months | ||
| let dayspassed = 0; | ||
| ['01', '02', '03', '04', '05', '06', '07', '08', '09', '10', '11', '12'] | ||
| .forEach((tms) => { | ||
| if (tms < monthstring) dayspassed += decimaldate.daysinmonth(yearstring, tms); | ||
| }); | ||
| dayspassed += parseInt(daystring); | ||
|
|
||
| // subtract 1 cuz day 0 is January 1 and not January 0 | ||
| // add 0.5 to get us 12 noon | ||
| dayspassed -= 1; | ||
| dayspassed += 0.5; | ||
|
|
||
| // divide by days in year, to get decimal portion since noon of Jan 1 | ||
| const dty = decimaldate.daysinyear(yearstring); | ||
| for (let m = 1; m < monthint; m++) { | ||
| const dtm = decimaldate.daysinmonth(yearint, m); | ||
| dayspassed += dtm; | ||
| } | ||
|
|
||
| // add the leftover days not in a prior month | ||
| // but minus 0.5 to get us to noon of the target day, as opposed to the end of the day | ||
| dayspassed = dayspassed + dayint - 0.5; | ||
|
|
||
| // divide by days in year, to get decimal portion | ||
| // even January 1 is 0.5 days in since we snap to 12 noon | ||
| const dty = decimaldate.daysinyear(yearint); | ||
| return dayspassed / dty; | ||
| }; |
There was a problem hiding this comment.
I don’t have a lot of context about why we calculate the proportion manually based on the number of days in the year. Wouldn’t it be more straightforward to generate two Date objects for January 1 of the current and next year and convert to milliseconds? I think this would be easier to reason about than something more manual. For example, we don’t have to care about leap years. If desired, we can still add half a day in there to snap to noon.
| // remove the artificial +1 that we add to make positive dates look intuitive | ||
| const truedecdate = decdate - 1; |
There was a problem hiding this comment.
Juggling two different versions of decimal dates is a red flag. While it might work, it means we’re probably doing some conversion just to convert back. I suspect a lot of this shifting is only necessary for the leap year computation, which is unnecessary if we rely more heavily on Date.
| const monthstring = monthint.toString().padStart(2, '0'); | ||
| const daystring = dayint.toString().padStart(2, '0'); | ||
| let yearstring; | ||
| if (yearint > 0) yearstring = yearint.toString().padStart(4, '0'); // just the year as 4 digits | ||
| else if (yearint == -1) yearstring = (Math.abs(yearint + 1).toString().padStart(4, '0')); // BCE offset by 1 but do not add a - sign | ||
| else yearstring = '-' + (Math.abs(yearint + 1).toString().padStart(4, '0')); // BCE offset by 1 and add - sign | ||
|
|
||
| return `${yearstring}-${monthstring}-${daystring}`; |
There was a problem hiding this comment.
It would be simpler to create a Date object and call toISOString. We don’t care about the time, so you can trim it at the T.
Note that, per ISO 8601, toISOString returns negative years that are six digits long, but we’re violating that part of the specification consistently throughout the stack. (Don’t worry about it just yet…) For now, we can correct for that situation with something like replace(/^-00(\d{4})/, "-$1").
Did you see the tests at https://github.com/OpenHistoricalMap/decimaldate-javascript/blob/master/tests.js ? Feel free to add more tests, and to report any tests that seem to fail. |
Oh, I missed that – sorry! Is there any chance we can get that hooked up to GitHub Actions or some other continuous integration tool so we can see the results when reviewing? That can be a separate task of course. |
There was a problem hiding this comment.
This PR does appear to address a subset of what we’re tracking in the linked issues. I’m in favor of landing these changes, if for no other reason than for OpenHistoricalMap/issues#624. However, I continue to believe we need to rewrite this code to rely on the browser for any calendrical operations. We are definitely overthinking it – this is what led to OpenHistoricalMap/issues#624 in the first place, and we definitely won’t be able to scale this approach to evaluate EDTF dates in the future. We can track simplification in OpenHistoricalMap/issues#562.
This PR also highlights the need to improve translation coverage of this library. We still haven’t set up a translation management system for anything but the main Rails codebase: OpenHistoricalMap/issues#470. In the meantime, maybe we should solicit translations directly in this repository from translators who are familiar with GitHub. Relying on browser APIs will automate some of the translatable strings, but some strings like “Change Date” will continue to need translation, unless we pursue a UI simplification as proposed in OpenHistoricalMap/issues#648.
| datepicker_month: "Mes", | ||
| datepicker_day: "Día", | ||
| datepicker_year: "Año", | ||
| datepicker_cebce: "a. C. o d. C.", | ||
| datepicker_submit_text: "Aplicar fecha", | ||
| datepicker_cancel_text: "Cancelar", | ||
| datepicker_title: "Cambiar fecha", | ||
| datepicker_format_text: "Formatos de fecha", | ||
| datepicker_text: "Entra una nueva fecha para actualizar la ubicación del mango y los datos que se muestran.", | ||
| months: ['Enero', 'Febrero', 'Marzo', 'Abril', 'Mayo', 'Junio', 'Julio', 'Agosto', 'Septiembre', 'Octubre', 'Noviembre', 'Diciembre'], |
There was a problem hiding this comment.
We can also eliminate this array by getting the month names from Intl.DateTimeFormat.formatToParts() just like you’ve done for era names. We’re doing this in iD as of OpenHistoricalMap/iD#185:
Let’s track this improvement as well in OpenHistoricalMap/issues#626.
|
When merging this PR of the updated decimaldate and slider UI changes, the DB's decimaldate functions should also be updated. See OpenHistoricalMap/issues#634 We should coordinate this, to minimize the time when the frontend and backend are using two different decimaldate functions. |
This brings two sets of improvements per feedback from Minh 1ec5
OpenHistoricalMap/issues#626
OpenHistoricalMap/issues#624
OpenHistoricalMap/issues#630
Testing
git clone https://github.com/OpenHistoricalMap/leaflet-ohm-timeslider-v2.gitgit checkout bce-iso8601-compliancepython -m SimpleHTTPServer 9646orpython3 -m http.server 9646