feat(map): display OpenHistoricalMap labels in user's preferred language#1056
Conversation
When the OpenHistoricalMap layer is selected, append the language parameter (?language=<locale>) to the style URL so that map labels are displayed in the user's preferred language matching the frontend locale setting. Fixes gramps-project#493
- Add OHM_LOCALE_MAP in util.js for mapping frontend locales (de_AT, en_GB, pt_BR, etc.) to OpenHistoricalMap-compatible ISO 639-1 codes (de, en, pt) - Add normalizeOhmLocale() function with explicit fallback chain - Add debug logging when OHM style is requested showing the locale mapping - Add comprehensive unit tests covering: - Base language code mapping - Regional variant normalization - Fallback behavior for undefined/null/empty/unknown locales - OHM_LOCALE_MAP completeness validation Fixes gramps-project#493
|
Thanks for looking into this! Great if it's that simple 🙂 |
There was a problem hiding this comment.
Pull request overview
Adds OpenHistoricalMap (OHM) label-language selection so the historical map layer’s labels align with the app’s current frontend locale, addressing issue #493.
Changes:
- Introduces
OHM_LOCALE_MAPandnormalizeOhmLocale()to normalize frontend locales (e.g.,de_AT) to OHM-compatible base codes (e.g.,de). - Updates
GrampsjsMapto append?language=<code>when selecting the OHM style. - Adds unit tests covering locale normalization and mapping completeness.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/components/GrampsjsMap.js |
Appends an OHM language query parameter based on current frontend language. |
src/util.js |
Adds locale mapping constant and normalization helper for OHM language codes. |
test/unit/mapOhmLocale.test.js |
Adds tests for locale normalization behavior and mapping coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use URL.searchParams.set() instead of string interpolation to properly handle OHM style URLs that may already contain query parameters. Fixes potential issue noted in PR review.
…uplicating Removes hardcoded frontendLanguages list from test file to avoid maintenance duplication. Now imports from src/strings.js directly.
Clarify the actual fallback chain in the JSDoc comment: 1. Falsy locale → 'en' 2. In OHM_LOCALE_MAP → use mapped value 3. Unknown locale with base code → extract base code 4. No base code available → 'en' Also add null to @param type since function handles it in practice.
|
Can you please close the comments once you address them with a reply what you did about them, so I don't have to dig through everything myself once more, thanks |
The Copilit comments are commented with the commit references. |
|
Thanks!
I know, but using this a lot recently (sometimes getting 3 or 4 copilot reviews on my own PRs) I know it's often quite random whether it picks up its previous commit and related changes or not. So better to have a human confirm it. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Oh hey, I didn’t realize you were working on this, glad to see it. In case you’re interested, I’m working on a new Diplomat plugin for localizing any MapLibre style. It’s a lot more thorough than what either of us have implemented for OHM so far. OpenHistoricalMap/openhistoricalmap-embed#39 would migrate to that plugin. Unfortunately it’s held up by a pesky font issue in the stylesheet, but only because I was looking to use Diplomat’s dual language labeling feature. If you don’t enable that, then it works just fine. |
That sounds great.
Even if off-topic here, if there is an issue about the font related blockage, I would offer support with reviewing end maybe helping to test or fix it. |
|
Diplomat is more or less ready to use. I’m just waiting for some feedback about the API design before releasing a v1.0. The font issue is OpenHistoricalMap/issues#1258. The fix might just be to drop the remote fonts from the stylesheet altogether. |
|
That sounds great! I think it might be better then to directly implement this rather than the partial solution in this PR. |
Forgot that I merged this already 😆 so let's ship the workaround and then switch to the better solution in the next release. |
|
I had to revert the change because it had no effect whatsoever - the query argument is ignored by OHM. @mahula did you test this locally? |
Motivation
OpenHistoricalMap (OHM) displays map labels in the default local contemporary language by default. Users expect map labels to appear in their preferred language matching the application's frontend language setting, as requested in issue #493 and the related forum topic.
Since OHM serves vector tiles, it supports on-the-fly language switching via the
?language=<ISO-639-code>URL parameter (see openhistoricalmap-embed#8). This PR implements that mechanism.Changes
Problem: Locale code mismatch
Frontend locales use regional variants (e.g.,
de_AT,en_GB,pt_BR) while OpenHistoricalMap only supports base ISO 639-1 codes (de,en,pt). Without normalization, users withde_ATlocale would not get German labels as OHM wouldn't recognize the code.Solution
Added
OHM_LOCALE_MAPinsrc/util.js— maps all frontend locales to their OHM-compatible base codes:de_AT→de,en_GB→en,pt_BR→pt, etc.Added
normalizeOhmLocale()insrc/util.js— converts frontend locale codes to OHM-compatible codes with explicit fallback chain:OHM_LOCALE_MAPsplit('_')[0])'en'for null/undefined/empty/unknownModified
_getStyleUrl()insrc/components/GrampsjsMap.js:?language=<locale>to the style URLHow to Test
Manual Testing
de-ATin browser settings)Supported Locales
The feature works with all frontend locales. Key regional variants that normalize correctly:
de_AT,de_DE→ German labelsen_GB,en_US→ English labelspt_BR,pt_PT→ Portuguese labelszh_CN,zh_HK,zh_TW→ Chinese labelsFallback Behavior
If a locale is not supported by OHM (e.g., a future locale not in the map), it falls back to the base code. If no base code is available, defaults to English.
Unit Tests
Run the new test suite:
npm test -- test/unit/mapOhmLocale.test.jsTests cover:
de→de)de_AT→de)undefined,null, empty stringFiles Changed
src/components/GrampsjsMap.js— added language parameter to OHM style URLsrc/util.js— addedOHM_LOCALE_MAPandnormalizeOhmLocale()test/unit/mapOhmLocale.test.js— new comprehensive test suiteRelated Issue
Fixes #493