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

i18n: userManager translation + some forgotten translations #557

Merged
merged 4 commits into from
Jul 16, 2023

Conversation

CamilleLegeron
Copy link
Collaborator

Hi,

I've added userManager translation + some forgotten translations

I've discovered that leaving a '+' in the string breaks the translation (see the AccessRules.ts file)

Copy link
Member

@dsagal dsagal left a comment

Choose a reason for hiding this comment

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

Thank you!

@@ -12,6 +12,10 @@ import {TableData} from 'app/common/TableData';
import {BaseFormatter} from 'app/common/ValueFormatter';
import {Computed, Disposable, Observable} from 'grainjs';
import debounce = require('lodash/debounce');
import { makeT } from 'app/client/lib/localization';

// Use 'tt' instade of 't', because 't' is already delared in the upper scope
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to use t here for consistency with all other code. The lint error about t being already declared is in unrelated lines which seem better candidates to rename the variable (e.g. one maps tables, so could replace t with table; and the other maps pages, so could replace t with page).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh your right, thanks ! I hadn't quite read linter's error, I changed it

@@ -103,13 +106,13 @@ export function showUserManagerModal(userApi: UserAPI, options: IUserManagerOpti
if (model.isSelfRemoved.get()) {
const name = resourceName(model.resourceType);
confirmModal(
`You are about to remove your own access to this ${name}`,
'Remove my access', tryToSaveChanges,
t(`You are about to remove your own access to this {{name}}`, { name }),
Copy link
Member

Choose a reason for hiding this comment

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

Renaming this from "name" to "resourceType" would be clearer I think. It may have values like "document", "workspace", or "team site".

description: t("Allow editors to edit structure (e.g. modify and delete tables, columns, " +
"layouts), and to write formulas, which give access to all data regardless of read restrictions."),
description: t("Allow editors to edit structure (e.g. modify and delete tables, columns, \
layouts), and to write formulas, which give access to all data regardless of read restrictions."),
Copy link
Member

Choose a reason for hiding this comment

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

I see that this is indeed missing from Weblate, and found some issues in i18next-scanner for this (i18next/i18next-scanner#35 and i18next/i18next-scanner#227). Thank you for finding and fixing!

Should the whitespace at the start of the continuation lines be removed? Otherwise the extra whitespace becomes part of the string.

BTW, it looks like there are a few other such instances of concatenated strings; I found some more using this command grep -rI '\bt(.*+$' ./app.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, it's a good point

'you will not be able to get it back without assistance ' +
`from someone else with sufficient access to the ${name}.`
t(`Once you have removed your own access, \
you will not be able to get it back without assistance \
Copy link
Member

Choose a reason for hiding this comment

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

Here too, it may be better to remove the leading whitespace on continuation lines, even if code ends up less well-aligned.

// If the user's access is inherited, give an explanation on how to change it.
dom.maybe((use) => use(inherited) && !isActiveUser, () => menuText(
`User inherits permissions from ${getResourceParent(this._model.resourceType)}. To remove, ` +
`set 'Inherit access' option to 'None'.`)),
t(`User inherits permissions from {{parent})}. To remove, \
Copy link
Member

Choose a reason for hiding this comment

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

I think an extra parenthesis crept in inside of })}.

`to resources inside. If removed here, this user will lose access to resources inside.`)),
this._model.isOrg ? menuText(`No default access allows access to be ` +
`granted to individual documents or workspaces, rather than the full team site.`) : null
t(`User has view access to {{ressource}} resulting from manually-set access \
Copy link
Member

Choose a reason for hiding this comment

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

The English version of "resource" has one "s".

@CamilleLegeron
Copy link
Collaborator Author

@dsagal the PR is ready and unconflicted again

@vviers vviers added the anct label Jul 11, 2023
Copy link
Member

@dsagal dsagal left a comment

Choose a reason for hiding this comment

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

Thank you!

@dsagal dsagal merged commit 61bd064 into gristlabs:main Jul 16, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants