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.

Sign up for GitHub

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

Jump to bottom

Refactor property value internationalization (fixes #3470) #3482

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Refactor property value internationalization (fixes #3470) #3482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea883b6
Refactor property value internationalization (fixes #3470)
jbaiter Jul 17, 2021
f54a6b3
Fix netlify preview
jbaiter Jul 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15,375 changes: 15,375 additions & 0 deletions __tests__/fixtures/version-2/i18n.json
View file
Open in desktop

Large diffs are not rendered by default.

48 changes: 48 additions & 0 deletions __tests__/integration/mirador/i18n.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
<meta name="theme-color" content="#000000">
<title>Mirador</title>
<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Roboto:300,400,500">
</head>
<body>
<div id="mirador" style="position: absolute; top: 0; bottom: 0; left: 0; right: 0;"></div>
<script>document.write("<script type='text/javascript' src='../../../dist/mirador.min.js?v=" + Date.now() + "'><\/script>");</script>
<script type="text/javascript">
var baseUrl = window.location.href.substring(
0, window.location.href.indexOf(window.location.pathname));
var miradorInstance = Mirador.viewer({
id: 'mirador',
language: 'de',
theme: {
transitions: window.location.port === '4488' ? { create: () => 'none' } : {},
},
windows: [{
manifestId: window. window.location.port
? `${baseUrl}/version-2/i18n.json`
: `${baseUrl}/__tests__/fixtures/version-2/i18n.json`,
thumbnailNavigationPosition: 'far-bottom',
showLocalePicker: true,
}],
catalog: [
{ manifestId: 'https://iiif.bodleian.ox.ac.uk/iiif/manifest/e32a277e-91e2-4a6d-8ba6-cc4bad230410.json' },
{ manifestId: 'https://iiif.harvardartmuseums.org/manifests/object/299843' },
{ manifestId: "https://media.nga.gov/public/manifests/nga_highlights.json", provider: "National Gallery of Art"},
{ manifestId: "https://data.ucd.ie/api/img/manifests/ucdlib:33064", provider: "Irish Architectural Archive"},
{ manifestId: "https://wellcomelibrary.org/iiif/b18035723/manifest", provider: "Wellcome Library"},
{ manifestId: "https://demos.biblissima.fr/iiif/metadata/florus-dispersus/manifest.json", provider: "Biblissima"},
{ manifestId: "https://www.e-codices.unifr.ch/metadata/iiif/gau-Fragment/manifest.json", provider: "e-codices - Virtual Manuscript Library of Switzerland"},
{ manifestId: "https://wellcomelibrary.org/iiif/collection/b18031511", provider: "Wellcome Library"},
{ manifestId: "https://gallica.bnf.fr/iiif/ark:/12148/btv1b10022508f/manifest.json", provider: "Bibliothèque nationale de France"},
{ manifestId: "https://manifests.britishart.yale.edu/Osbornfa1", provider: "Beinecke Rare Book and Manuscript Library, Yale University"},
{ manifestId: "https://iiif.biblissima.fr/chateauroux/B360446201_MS0005/manifest.json", provider: "Biblissima"},
{ manifestId: "https://iiif.durham.ac.uk/manifests/trifle/32150/t1/m4/q7/t1m4q77fr328/manifest", provider: "Durham University Library"},
//{ manifestId: "https://iiif.vam.ac.uk/collections/O1023003/manifest.json", provider: "Ocean liners"},
{ manifestId: "https://zavicajna.digitalna.rs/iiif/iiif/api/presentation/2/4aa44ad1-0b74-4590-ab09-534a38cb7c53%252F00000001%252Fostalo01%252F00000012/manifest", provider: "Библиотека 'Милутин Бојић'"},
]
});
</script>
</body>
</html>
9 changes: 0 additions & 9 deletions __tests__/src/selectors/manifests.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import manifestFixtureWithAProvider from '../../fixtures/version-3/with_a_provid
import manifestFixtureFg165hz3589 from '../../fixtures/version-2/fg165hz3589.json';
import {
getManifestoInstance,
getManifestLocale,
getDestructuredMetadata,
getManifestStatus,
getManifestLogo,
Expand Down Expand Up @@ -282,14 +281,6 @@ describe('getManifestMetadata', () => {
});
});

describe('getManifestLocale', () => {
it('gets the default locale for the manifest', () => {
const state = { manifests: { x: { json: manifestFixture002 } } };
const received = getManifestLocale(state, { manifestId: 'x' });
expect(received).toEqual('en');
});
});

describe('getMetadataLocales', () => {
it('gets the locales preseent in the manifest metadata', () => {
const manifest = {
Expand Down
15 changes: 9 additions & 6 deletions src/components/AppProviders.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,20 +61,22 @@ export class AppProviders extends Component {
/**
* Set i18n language on component mount
*/
componentDidMount() {
const { language } = this.props;
async componentDidMount() {
const { language, updateUserLanguages } = this.props;

this.i18n.changeLanguage(language);
await this.i18n.changeLanguage(language);
updateUserLanguages(this.i18n.languages);
}

/**
* Update the i18n language if it is changed
*/
componentDidUpdate(prevProps) {
const { language } = this.props;
async componentDidUpdate(prevProps) {
const { language, updateUserLanguages } = this.props;

if (prevProps.language !== language) {
this.i18n.changeLanguage(language);
await this.i18n.changeLanguage(language);
updateUserLanguages(this.i18n.languages);
}
}

Expand Down Expand Up @@ -127,6 +129,7 @@ AppProviders.propTypes = {
setWorkspaceFullscreen: PropTypes.func.isRequired,
theme: PropTypes.object.isRequired, // eslint-disable-line react/forbid-prop-types
translations: PropTypes.object.isRequired, // eslint-disable-line react/forbid-prop-types
updateUserLanguages: PropTypes.func.isRequired,
};

AppProviders.defaultProps = {
Expand Down
8 changes: 5 additions & 3 deletions src/components/CanvasLayers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ const reorder = (list, startIndex, endIndex) => {
/** */
export class CanvasLayers extends Component {
/** */
static getUseableLabel(resource, index) {
static getUseableLabel(resource, index, langs) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

return (resource
&& resource.getLabel
&& resource.getLabel().length > 0)
? resource.getLabel().getValue()
? resource.getLabel().getValue(langs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

: String(index + 1);
}

Expand Down Expand Up @@ -117,6 +117,7 @@ export class CanvasLayers extends Component {
renderLayer(resource, index) {
const {
classes,
languages,
layerMetadata,
t,
} = this.props;
Expand All @@ -143,7 +144,7 @@ export class CanvasLayers extends Component {
component="div"
variant="body1"
>
{CanvasLayers.getUseableLabel(resource, index)}
{CanvasLayers.getUseableLabel(resource, index, languages)}
<div>
<MiradorMenuButton aria-label={t(layer.visibility ? 'layer_hide' : 'layer_show')} edge="start" size="small" onClick={() => { this.setLayerVisibility(resource.id, !layer.visibility); }}>
{ layer.visibility ? <VisibilityIcon /> : <VisibilityOffIcon /> }
Expand Down Expand Up @@ -265,6 +266,7 @@ CanvasLayers.propTypes = {
classes: PropTypes.objectOf(PropTypes.string),
index: PropTypes.number.isRequired,
label: PropTypes.string.isRequired,
languages: PropTypes.arrayOf(PropTypes.string).isRequired,
layerMetadata: PropTypes.objectOf(PropTypes.shape({
opacity: PropTypes.number,
})),
Expand Down
18 changes: 10 additions & 8 deletions src/components/CollectionDialog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ import ManifestInfo from '../containers/ManifestInfo';
*/
export class CollectionDialog extends Component {
/** */
static getUseableLabel(resource, index) {
static getUseableLabel(resource, langs) {
return (resource
&& resource.getLabel
&& resource.getLabel().length > 0)
? resource.getLabel().getValue()
: String(index + 1);
? resource.getLabel().getValue(langs)
: resource.id;
}

/** */
Expand Down Expand Up @@ -142,6 +142,7 @@ export class CollectionDialog extends Component {
collection,
error,
isMultipart,
languages,
manifest,
ready,
t,
Expand All @@ -164,7 +165,7 @@ export class CollectionDialog extends Component {
const requiredStatement = manifest
&& asArray(manifest.getRequiredStatement()).filter(l => l.getValue()).map(labelValuePair => ({
label: null,
values: labelValuePair.getValues(),
values: labelValuePair.getValues(languages),
}));

const collections = manifest.getCollections();
Expand All @@ -184,7 +185,7 @@ export class CollectionDialog extends Component {
{ t(isMultipart ? 'multipartCollection' : 'collection') }
</Typography>
<Typography variant="h3">
{CollectionDialog.getUseableLabel(manifest)}
{CollectionDialog.getUseableLabel(manifest, languages)}
</Typography>
</DialogTitle>
<ScrollIndicatedDialogContent className={classes.dialogContent}>
Expand All @@ -193,7 +194,7 @@ export class CollectionDialog extends Component {
startIcon={<ArrowBackIcon />}
onClick={() => this.goToPreviousCollection()}
>
{CollectionDialog.getUseableLabel(collection)}
{CollectionDialog.getUseableLabel(collection, languages)}
</Button>
)}

Expand Down Expand Up @@ -239,7 +240,7 @@ export class CollectionDialog extends Component {
onClick={() => { this.selectCollection(c); }}
className={classes.collectionItem}
>
{CollectionDialog.getUseableLabel(c)}
{CollectionDialog.getUseableLabel(c, languages)}
</MenuItem>
))
}
Expand All @@ -254,7 +255,7 @@ export class CollectionDialog extends Component {
onClick={() => { this.selectManifest(m); }}
className={classes.collectionItem}
>
{CollectionDialog.getUseableLabel(m)}
{CollectionDialog.getUseableLabel(m, languages)}
</MenuItem>
))
}
Expand All @@ -280,6 +281,7 @@ CollectionDialog.propTypes = {
error: PropTypes.string,
hideCollectionDialog: PropTypes.func.isRequired,
isMultipart: PropTypes.bool,
languages: PropTypes.arrayOf(PropTypes.string).isRequired,
manifest: PropTypes.object.isRequired, // eslint-disable-line react/forbid-prop-types
manifestId: PropTypes.string.isRequired,
ready: PropTypes.bool,
Expand Down
11 changes: 6 additions & 5 deletions src/components/IIIFThumbnail.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ import getThumbnail from '../lib/ThumbnailFactory';
*/
export class IIIFThumbnail extends Component {
/** */
static getUseableLabel(resource, index) {
static getUseableLabel(resource, langs) {
return (resource
&& resource.getLabel
&& resource.getLabel().length > 0)
? resource.getLabel().getValue()
: String(index + 1);
? resource.getLabel().getValue(langs)
: resource.id;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why resource.id instead of the index value?

Copy link
Collaborator Author

@jbaiter jbaiter Aug 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}

/**
Expand Down Expand Up @@ -129,9 +129,9 @@ export class IIIFThumbnail extends Component {

/** */
label() {
const { label, resource } = this.props;
const { label, resource, languages } = this.props;

return label || IIIFThumbnail.getUseableLabel(resource);
return label || IIIFThumbnail.getUseableLabel(resource, languages);
}

/**
Expand Down Expand Up @@ -180,6 +180,7 @@ IIIFThumbnail.propTypes = {
imagePlaceholder: PropTypes.string,
label: PropTypes.string,
labelled: PropTypes.bool,
languages: PropTypes.arrayOf(PropTypes.string).isRequired,
maxHeight: PropTypes.number,
maxWidth: PropTypes.number,
resource: PropTypes.object.isRequired, // eslint-disable-line react/forbid-prop-types
Expand Down
10 changes: 9 additions & 1 deletion src/components/LocalePicker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,17 @@ export class LocalePicker extends Component {
const {
availableLocales,
classes,
locale,
setLocale,
userLanguages,
} = this.props;
let { locale } = this.props;

if (!setLocale || availableLocales.length < 2) return <></>;
// 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];
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

}
return (
<FormControl>
<Select
Expand Down Expand Up @@ -55,11 +61,13 @@ LocalePicker.propTypes = {
classes: PropTypes.objectOf(PropTypes.string),
locale: PropTypes.string,
setLocale: PropTypes.func,
userLanguages: PropTypes.arrayOf(PropTypes.string),
};

LocalePicker.defaultProps = {
availableLocales: [],
classes: {},
locale: '',
setLocale: undefined,
userLanguages: [],
};
12 changes: 9 additions & 3 deletions src/components/SidebarIndexList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@ import SidebarIndexThumbnail from '../containers/SidebarIndexThumbnail';
export class SidebarIndexList extends Component {
/** @private */
getIdAndLabelOfCanvases() {
const { canvases } = this.props;
const { canvases, languages } = this.props;

return canvases.map((canvas, index) => ({
id: canvas.id,
label: new MiradorCanvas(canvas).getLabel(),
label: new MiradorCanvas(canvas).getLabel(languages),
}));
}

Expand All @@ -25,6 +25,7 @@ export class SidebarIndexList extends Component {
canvases,
classes,
containerRef,
languages,
selectedCanvasIds,
setCanvas,
variant,
Expand Down Expand Up @@ -64,7 +65,11 @@ export class SidebarIndexList extends Component {
component="li"
selected={selectedCanvasIds.includes(canvas.id)}
>
<Item label={canvas.label} canvas={canvases[canvasIndex]} />
<Item
label={canvas.label}
canvas={canvases[canvasIndex]}
languages={languages}
/>
</MenuItem>
</ScrollTo>
);
Expand All @@ -79,6 +84,7 @@ SidebarIndexList.propTypes = {
canvases: PropTypes.array.isRequired, // eslint-disable-line react/forbid-prop-types
classes: PropTypes.objectOf(PropTypes.string).isRequired,
containerRef: PropTypes.oneOf([PropTypes.func, PropTypes.object]).isRequired,
languages: PropTypes.arrayOf(PropTypes.string).isRequired,
selectedCanvasIds: PropTypes.arrayOf(PropTypes.string),
setCanvas: PropTypes.func.isRequired,
variant: PropTypes.oneOf(['item', 'thumbnail']),
Expand Down
10 changes: 7 additions & 3 deletions src/components/SidebarIndexTableOfContents.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export class SidebarIndexTableOfContents extends Component {
}

/** */
buildTreeItems(nodes, visibleNodeIds, containerRef, nodeIdToScrollTo) {
buildTreeItems(nodes, visibleNodeIds, containerRef, nodeIdToScrollTo, languages) {
const { classes } = this.props;
if (!nodes) {
return null;
Expand Down Expand Up @@ -88,7 +88,7 @@ export class SidebarIndexTableOfContents extends Component {
[classes.visibleNode]: visibleNodeIds.indexOf(node.id) !== -1,
})}
>
{node.label}
{node.data?.getLabel()?.getValue(languages)}
</div>
)}
onClick={() => this.selectTreeItem(node)}
Expand All @@ -110,6 +110,7 @@ export class SidebarIndexTableOfContents extends Component {
render() {
const {
classes, treeStructure, visibleNodeIds, expandedNodeIds, containerRef, nodeIdToScrollTo,
languages,
} = this.props;

if (!treeStructure) {
Expand All @@ -125,7 +126,9 @@ export class SidebarIndexTableOfContents extends Component {
defaultEndIcon={<></>}
expanded={expandedNodeIds}
>
{this.buildTreeItems(treeStructure.nodes, visibleNodeIds, containerRef, nodeIdToScrollTo)}
{this.buildTreeItems(
treeStructure.nodes, visibleNodeIds, containerRef, nodeIdToScrollTo, languages,
)}
</TreeView>
</>
);
Expand All @@ -139,6 +142,7 @@ SidebarIndexTableOfContents.propTypes = {
PropTypes.shape({ current: PropTypes.instanceOf(Element) }),
]).isRequired,
expandedNodeIds: PropTypes.arrayOf(PropTypes.string).isRequired,
languages: PropTypes.arrayOf(PropTypes.string).isRequired,
nodeIdToScrollTo: PropTypes.func.isRequired,
setCanvas: PropTypes.func.isRequired,
toggleNode: PropTypes.func.isRequired,
Expand Down
Loading