Refactor property value internationalization (fixes #3470) #3482

jbaiter · 2021-07-17T22:08:30Z

This admittedly huge changeset refactors the way the correct language value for a given property value is determined. A recent change in the Manifesto library made it possible to pass a list of languages (in descending order of preference) to the PropertyValue.getValue and PropertyValue.getValues methods, instead of relying on a single default locale that would be set when parsing the manifest. This allows for a better user experience, since we can now dynamically respond to changes in the user's language preferences without having to re-parse the manifest. Additionally, we can make use of the browser's language preferences to better match the user's language abilities.

However, this required a substantial reworking of quite a few parts of Mirador, namely:

Introduced a new config.userLanguages redux state variable that has the list of languages in descending order of preference (as returned by i18next.languages after setting the configured language)
Every selector that would previously simply call getValue on a property value was changed to retrieve the user languages from the store and pass them to getValue. Additionally, for property values that are displayed in the companion window, where the locale can be overridden separately from the global language, selectors are extended to take a third overrideLanguages parameter that allows customization of the languages passed to getValue/getValues
When changing the global language setting, the value in the companion window's LocalePicker (if enabled) now will select the next-best available language according to the langauge pereferences, if the desired language is not part of the metadata.

As a whole, these changes now allow on-the-fly switching of languages via both the global language selector and the companion window locale picker for all property values parsed from the manifest, with support for falling back according to the user's client language preferences.

I'd be grateful for a careful review, I had quite a bit of trouble keeping all the parts involved in my head, there might be things I have missed or not done as elegant as they could be.

The changes can be tested in this new integration test:
https://deploy-preview-3482--mirador-dev.netlify.app/__tests__/integration/mirador/i18n.html

codecov-commenter · 2021-07-17T22:28:43Z

Codecov Report

Merging #3482 (ea883b6) into master (c941b70) will decrease coverage by 0.10%.
The diff coverage is 90.27%.

@@            Coverage Diff             @@
##           master    #3482      +/-   ##
==========================================
- Coverage   88.91%   88.80%   -0.11%     
==========================================
  Files         198      198              
  Lines        3409     3439      +30     
==========================================
+ Hits         3031     3054      +23     
- Misses        378      385       +7

Impacted Files	Coverage Δ
src/components/CanvasLayers.js	`100.00% <ø> (ø)`
src/components/CollectionDialog.js	`51.11% <ø> (ø)`
src/components/SidebarIndexTableOfContents.js	`100.00% <ø> (ø)`
src/components/SidebarIndexThumbnail.js	`100.00% <ø> (ø)`
src/components/WindowSideBarCollectionPanel.js	`5.00% <ø> (ø)`
src/components/WindowSideBarInfoPanel.js	`80.00% <ø> (ø)`
src/lib/MiradorCanvas.js	`97.67% <ø> (ø)`
src/state/actions/action-types.js	`100.00% <ø> (ø)`
src/state/actions/config.js	`83.33% <0.00%> (-16.67%)`	⬇️
src/state/reducers/config.js	`88.88% <0.00%> (-11.12%)`	⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c941b70...ea883b6. Read the comment docs.

This admittedly huge changeset refactors the way the correct language value for a given property value is determined. A recent change in the Manifesto library made it possible to pass a list of languages (in descending order of preference) to the `PropertyValue.getValue` and `PropertyValue.getValues` methods, instead of relying on a single default locale that would be set when parsing the manifest. This allows for a better user experience, since we can now dynamically respond to changes in the user's language preferences without having to re-parse the manifest. Additionally, we can make use of the browser's language preferences to better match the user's language abilities. However, this required a substantial reworking of quite a few parts of Mirador, namely: - Introduced a new `config.userLanguages` redux state variable that has the list of languages in descending order of preference (as returned by i18next.languages after setting the configured language) - Every selector that would previously simply call `getValue` on a property value was changed to retrieve the user languages from the store and pass them to `getValue`. Additionally, for property values that are displayed in the companion window, where the locale can be overridden separately from the global language, selectors are extended to take a third `overrideLanguages` parameter that allows customization of the languages passed to `getValue`/`getValues` - When changing the global language setting, the value in the companion window's `LocalePicker` (if enabled) now will select the next-best available language according to the langauge pereferences, if the desired language is not part of the metadata. As a whole, these changes now allow on-the-fly switching of languages via both the global language selector and the companion window locale picker for all property values parsed from the manifest, with support for falling back according to the user's client language preferences.

cbeer · 2021-08-02T14:25:14Z

src/components/CanvasLayers.js

    return (resource
      && resource.getLabel
      && resource.getLabel().length > 0)
-      ? resource.getLabel().getValue()
+      ? resource.getLabel().getValue(langs)


Can you remind me of the manifesto behavior if there's no label in a given language? Is there a fallback or something?

Currently the behavior is to simply pick the first value, with no regard for the (legacy) defaultLocale. I'm still planning on submitting a PR that uses that instead, but haven't gotten around to it yet.

cbeer · 2021-08-02T14:27:51Z

src/components/CanvasLayers.js

@@ -30,11 +30,11 @@ const reorder = (list, startIndex, endIndex) => {
 /** */
 export class CanvasLayers extends Component {
  /** */
-  static getUseableLabel(resource, index) {
+  static getUseableLabel(resource, index, langs) {


This probably goes for all these label helpers... I wonder if we should refactor these back into the instance, now that they depend on some state data?

Probably a good idea, yes! Or maybe refactor it into a utility function? I've seen this exact static method (and minor variations of it) in more than a handful components.

cbeer · 2021-08-02T14:29:59Z

src/components/IIIFThumbnail.js

-      ? resource.getLabel().getValue()
-      : String(index + 1);
+      ? resource.getLabel().getValue(langs)
+      : resource.id;


Why resource.id instead of the index value?

~~Sorry about that, it was probably an ill-advised attempt a a unification of the various getUsableLabel methods that went nowhere in the end, I'll revert!~~

Just checked, index was never passed in this component, so the fallback would always be "NaN" (String(undefined + 1)). Using the resource identifier seems more sensible.

cbeer · 2021-08-02T14:31:56Z

src/components/LocalePicker.js

+    // If `locale` is not among the available locales, it should be the first available
+    // locale that matches the language preferences from the `userLanguages` store value
+    if (availableLocales.indexOf(locale) < 0) {
+      locale = userLanguages.find(l => availableLocales.indexOf(l) >= 0) ?? availableLocales[0];


Should we push a default fallback locale into settings? It probably shouldn't happen, but 🤷

Well, there's already the language value in the store that determines the language of the whole app, which is included in the userLanguages list. I'm not sure if an additional metadataFallbackLocale really makes sense, wouldn't it be identical to language in most usage scenarios?

jbaiter · 2021-11-19T14:54:58Z

Any update on this?

jbaiter · 2022-09-20T14:21:27Z

It's been over a year, any update on this? If this is still considered relevant, I can rebase my changes on top of the latest changes.

jbaiter force-pushed the propval-i18n-refactor branch from f28b81c to 84298aa Compare July 17, 2021 22:23

jbaiter force-pushed the propval-i18n-refactor branch 2 times, most recently from c0102d0 to f4152fd Compare July 17, 2021 22:30

jbaiter requested a review from cbeer July 18, 2021 16:37

jbaiter force-pushed the propval-i18n-refactor branch from f4152fd to ea883b6 Compare July 19, 2021 07:47

Fix netlify preview

f54a6b3

cbeer reviewed Aug 2, 2021

View reviewed changes

jbaiter requested a review from cbeer August 6, 2021 15:35

camillevilla self-requested a review October 7, 2022 22:38

cbeer mentioned this pull request Jan 10, 2024

Refactor manifesto initialization to push property localization down #3871

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor property value internationalization (fixes #3470) #3482

Refactor property value internationalization (fixes #3470) #3482

jbaiter commented Jul 17, 2021 •

edited

Loading

codecov-commenter commented Jul 17, 2021 •

edited

Loading

cbeer Aug 2, 2021

jbaiter Aug 6, 2021

cbeer Aug 2, 2021

jbaiter Aug 6, 2021

cbeer Aug 2, 2021

jbaiter Aug 6, 2021 •

edited

Loading

cbeer Aug 2, 2021

jbaiter Aug 6, 2021

jbaiter commented Nov 19, 2021

jbaiter commented Sep 20, 2022

Refactor property value internationalization (fixes #3470) #3482

Are you sure you want to change the base?

Refactor property value internationalization (fixes #3470) #3482

Conversation

jbaiter commented Jul 17, 2021 • edited Loading

codecov-commenter commented Jul 17, 2021 • edited Loading

Codecov Report

cbeer Aug 2, 2021

Choose a reason for hiding this comment

jbaiter Aug 6, 2021

Choose a reason for hiding this comment

cbeer Aug 2, 2021

Choose a reason for hiding this comment

jbaiter Aug 6, 2021

Choose a reason for hiding this comment

cbeer Aug 2, 2021

Choose a reason for hiding this comment

jbaiter Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

cbeer Aug 2, 2021

Choose a reason for hiding this comment

jbaiter Aug 6, 2021

Choose a reason for hiding this comment

jbaiter commented Nov 19, 2021

jbaiter commented Sep 20, 2022

jbaiter commented Jul 17, 2021 •

edited

Loading

codecov-commenter commented Jul 17, 2021 •

edited

Loading

jbaiter Aug 6, 2021 •

edited

Loading