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

Add date field type #185

Merged
merged 1 commit into from
Dec 20, 2023
Merged

Add date field type #185

merged 1 commit into from
Dec 20, 2023

Conversation

1ec5
Copy link
Member

@1ec5 1ec5 commented Dec 12, 2023

The Start Date and End Date fields now consist of structured input fields for each date component instead of a freeform text field. The combo box options and the arrangement of input fields is determined by the locale. A separate era combo box means the user doesn’t need to know that BCE years are represented by negative years offset by one in the underlying tag (OpenHistoricalMap/issues#624).

English Japanese German

Fixes OpenHistoricalMap/issues#563 and fixes OpenHistoricalMap/issues#644.

@1ec5 1ec5 self-assigned this Dec 12, 2023
return monthPart && monthPart.value;
}

let monthNames = Array.from({length: 12}, (_, i) => getMonthName(i, 'long'))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The long month format isn’t great for Chinese. It winds up spelling out the month number.

}


function change() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fields don’t seem to be updating when undoing and redoing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this as well when testing locally. Maybe some change is needed in https://github.com/OpenHistoricalMap/iD/blob/518546cc5d4331e148a1718e66d44adf6bb78079/modules/core/history.js? But no idea what change! Do you think the improved date entry experience is worth merging and deploying this without undo working (and then fix undo separately)? I could see arguments either way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ab49980.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing, now the numeric element (day) does clear on undo, and in cases where there's a value for month and I change it, the month reverts (as expected). But in cases where month is blank and I add a value, the value stays after I hit undo.

Here's a screen cap of that https://recordit.co/OT5NNxLWoA

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0671357.

Comment on lines +46 to +47
let bceName = getEraName(0, 'short');
let ceName = getEraName(1, 'short');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, Intl.DateTimeFormat only exposes the locale’s default era names. Ideally we’d be using the secular era names, matching best practice in the relevant academic fields, but CLDR lets individual locales decide whether to provide secular names as the era names or variant era names, and ECMAScript doesn’t provide access to the variant era names. We might have to specify our own localizable strings for the two eras, even though many locales would have secular era names by default anyways.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened tc39/ecma402#845 to request an addition to ECMAScript that would enable us to use the variant era names in CLDR. In the meantime, we’ll have to either live with the BC/AD names for now or maintain our own localizable strings. It’s no big deal to add more strings to core.yaml, but they won’t actually get translated because we haven’t set up a Translatewiki.net project for iD yet: OpenHistoricalMap/issues#470.

case 'day':
dayInput = dayInput.enter()
.append('input')
.attr('type', 'number')
Copy link
Member Author

@1ec5 1ec5 Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The year and day fields will need to be reworked a bit for consistency once we merge in the number formatting improvements in openstreetmap#8769.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants