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

In Facility task cards, only quote the facility name and not the ID fragment #8093

Merged
merged 1 commit into from
May 20, 2021
Merged

In Facility task cards, only quote the facility name and not the ID fragment #8093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion kolibri/core/assets/src/mixins/commonCoreStrings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ export const coreStrings = createTranslator('CommonCoreStrings', {
invalidCredentialsError: 'Incorrect username or password',

// Formatting
nameWithIdInParens: '{name} ({id})',
nameWithIdInParens: `'{name}' ({id})`,
quotedPhrase: `'{phrase}'`,
dashSeparatedPair: '{item1} - {item2}',
dashSeparatedTriple: '{item1} - {item2} - {item3}',
Expand Down
6 changes: 3 additions & 3 deletions kolibri/core/assets/src/mixins/taskStrings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ const taskStrings = createTranslator('TaskStrings', {
context: 'Sync task status',
},
syncFacilityTaskLabel: {
message: "Sync '{facilityName}'",
message: 'Sync {facilityName}',
Copy link
Contributor

Choose a reason for hiding this comment

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

This change and similar ones looks incorrect to me. From the design system:

Always wrap user-generated text in 'single quotes'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

facilityName is interpolated to something like 'My facility' (a1b2), so the full message should match what you're saying. Check out the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, interesting – I didn't realize that facilityNameWithId was used inside other strings. I think that this pattern, while DRY, is technically incorrect for i18n strings because we're essentially using string concatenation to join two incomplete phrases.

A more technically correct string would include both inputs:

     message: `Sync '{facilityName}' ({id})`,

My understanding is that this less-DRY format is easier for the translators to work with effectively.

(cc @radinamatic in case you have guidance and @jredrejo from the original change)

None of this is super critical – just wanted to flag for discussion and future work.

context: 'Description of sync-facility task',
},
syncStepAndDescription: {
Expand All @@ -87,7 +87,7 @@ const taskStrings = createTranslator('TaskStrings', {
context: 'Remove facility task status',
},
removeFacilityTaskLabel: {
message: "Remove '{facilityName}'",
message: 'Remove {facilityName}',
context: 'Description of a remove-facility task',
},
removeFacilitySuccessStatus: {
Expand All @@ -97,7 +97,7 @@ const taskStrings = createTranslator('TaskStrings', {

// Import Facility Task strings
importFacilityTaskLabel: {
message: "Import '{facilityName}'",
message: 'Import {facilityName}',
context: 'Description of import-facility task',
},
importSuccessStatus: {
Expand Down
10 changes: 5 additions & 5 deletions kolibri/plugins/device/assets/src/views/__test__/syncTaskUtils.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,14 @@ describe('syncTaskUtils.syncFacilityTaskDisplayInfo', () => {
const task = makeTask('RUNNING');
task.type = 'SYNCPEER/FULL';
const displayInfo = syncFacilityTaskDisplayInfo(task);
expect(displayInfo.headingMsg).toEqual("Sync 'generic facility (fac1)'");
expect(displayInfo.headingMsg).toEqual("Sync 'generic facility' (fac1)");
});

it('displays the correct header for facility-import tasks', () => {
const task = makeTask('RUNNING');
task.type = 'SYNCPEER/PULL';
const displayInfo = syncFacilityTaskDisplayInfo(task);
expect(displayInfo.headingMsg).toEqual("Import 'generic facility (fac1)'");
expect(displayInfo.headingMsg).toEqual("Import 'generic facility' (fac1)");
});

it('display title, started by username, and device name are invariant wrt status', () => {
Expand All @@ -63,9 +63,9 @@ describe('syncTaskUtils.syncFacilityTaskDisplayInfo', () => {
ALL_STATUSES.forEach(status => {
task.status = status;
expect(syncFacilityTaskDisplayInfo(task)).toMatchObject({
headingMsg: "Sync 'invariant facility (fac1)'",
headingMsg: "Sync 'invariant facility' (fac1)",
startedByMsg: "Started by 'invariant user'",
deviceNameMsg: "'invariant device (dev1)'",
deviceNameMsg: "'invariant device' (dev1)",
});
});
});
Expand Down Expand Up @@ -164,7 +164,7 @@ describe('syncTaskUtils.removeFacilityTaskDisplayInfo', () => {
ALL_STATUSES.forEach(status => {
const task = makeTask(status);
expect(removeFacilityTaskDisplayInfo(task)).toMatchObject({
headingMsg: "Remove 'removed facility (fac1)'",
headingMsg: "Remove 'removed facility' (fac1)",
startedByMsg: "Started by 'removing user'",
});
});
Expand Down
4 changes: 1 addition & 3 deletions kolibri/plugins/device/assets/src/views/syncTaskUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,7 @@ export function syncFacilityTaskDisplayInfo(task) {
if (task.type === TaskTypes.SYNCDATAPORTAL) {
deviceNameMsg = 'Kolibri Data Portal';
} else if (task.device_name) {
deviceNameMsg = coreString('quotedPhrase', {
phrase: formatNameWithId(task.device_name, task.device_id),
});
deviceNameMsg = formatNameWithId(task.device_name, task.device_id);
}
const syncStep = syncTaskStatusToStepMap[task.sync_state];
const statusDescription =
Expand Down