Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Refactor delete message translation #992

Merged

Conversation

baoqchau
Copy link
Contributor

Fixes #543.

Changes proposed in this pull request:

  • Added 3 standard delete message in translation file.
  • Replace "delete" message in multiple template files with new standard message.

cc @HospitalRun/core-maintainers

@baoqchau baoqchau closed this Mar 15, 2017
@baoqchau baoqchau reopened this Mar 15, 2017
@baoqchau baoqchau closed this Mar 16, 2017
@baoqchau baoqchau force-pushed the refactor-delete-message-translation branch from 228a748 to 24665f1 Compare March 16, 2017 02:38
@baoqchau baoqchau reopened this Mar 16, 2017
@@ -25,7 +25,7 @@ export default AbstractIndexRoute.extend(ModalHelper, UserSession, {
actions: {
deleteItem(item) {
let i18n = this.get('i18n');
let message = i18n.t('admin.customForms.messages.deleteForm');
let message = i18n.t('messages.delete_singular', { name: 'form' });
Copy link
Member

Choose a reason for hiding this comment

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

This code is passing a non localized string when it should be passing in a localized string or it should use the name of the item being deleted.

@@ -348,7 +348,7 @@ export default Ember.Controller.extend(BillingCategories, EKMixin,
confirmDeleteValue(value) {
let i18n = this.get('i18n');
let title = i18n.t('admin.lookup.titles.deleteLookupValue');
let message = i18n.t('admin.lookup.messages.deleteLookupValue', { value });
let message = i18n.t('messages.delete_singular', { name: value.concat(' value') });
Copy link
Member

Choose a reason for hiding this comment

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

This code is passing a non localized string when it should be passing in a localized string or it should use the name of the item being deleted.

@@ -5,6 +5,6 @@
updateButtonText=updateButtonText }}
<div class="alert alert-danger">
<span class="glyphicon glyphicon-warning-sign"></span>
{{t 'appointments.messages.deleteAppointmentMessage'}}
{{t 'messages.delete_singular' name='appointment'}}
Copy link
Member

Choose a reason for hiding this comment

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

This code is passing a non localized string when it should be passing in a localized string or it should use the name of the item being deleted.

Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

@baoqchau thanks for the PR! It looks like most of the changes you have made are passing a non localized string to i18n.t (eg passing in the word 'form'). You need to either change those to use a localized string (eg localized version of 'form' or pass in the name of the item you are deleting.

 delete: 'Are you sure you wish to delete {{name}}?'

Should be used when passing in a name
and when using

    delete_singular: 'Are you sure you wish to delete this{{name}}?'
    delete_plural: 'Are you sure you wish to delete {{name}}?'

You need to pass in a localized value for name.

@@ -5,6 +5,6 @@
updateButtonText=updateButtonText }}
<div class="alert alert-danger">
<span class="glyphicon glyphicon-warning-sign"></span>
{{t 'imaging.messages.delete'}}
{{t 'messages.delete_singular' name='imaging request' }}
Copy link
Member

Choose a reason for hiding this comment

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

This code is passing a non localized string when it should be passing in a localized string or it should use the name of the item being deleted.

@@ -5,6 +5,6 @@
updateButtonText=updateButtonText }}
<div class="alert alert-danger">
<span class="glyphicon glyphicon-warning-sign"></span>
{{t 'incident.messages.deleteCategory' name=model.incidentCategoryName}}
{{t 'messages.delete' name=model.incidentCategoryName}}
Copy link
Member

Choose a reason for hiding this comment

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

This example is using an item name and is done correctly

@@ -36,7 +36,7 @@ export default AbstractEditController.extend({

showDeleteItem(item) {
let i18n = get(this, 'i18n');
let message = i18n.t('incident.messages.deleteItem');
let message = i18n.t('messages.delete_singular', { name: 'item' });
Copy link
Member

Choose a reason for hiding this comment

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

This code is passing a non localized string when it should be passing in a localized string or it should use the name of the item being deleted.

@@ -4,6 +4,6 @@
updateButtonAction=updateButtonAction
updateButtonText=updateButtonText}}
<div class="alert alert-danger">
<span class="glyphicon glyphicon-warning-sign"></span> {{t 'incident.messages.deleteIncident' name=name}}
<span class="glyphicon glyphicon-warning-sign"></span> {{t 'messages.delete_singular' name='incident'}}
Copy link
Member

Choose a reason for hiding this comment

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

This code is passing a non localized string when it should be passing in a localized string or it should use the name of the item being deleted.

@@ -205,7 +205,7 @@ export default AbstractEditController.extend(IncidentStatuses, FriendlyId, Patie

showDeleteAttachment(attachment) {
let i18n = get(this, 'i18n');
let message = i18n.t('incident.messages.deleteAttachment');
let message = i18n.t('messages.delete_singular', { name: 'attachment' });
Copy link
Member

Choose a reason for hiding this comment

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

This code is passing a non localized string when it should be passing in a localized string or it should use the name of the item being deleted.

@@ -83,7 +83,7 @@ export default AbstractEditController.extend(ChargeActions, PatientSubmodule, {
closeModalOnConfirm: false,
confirmAction: 'deleteCharge',
title: this.get('i18n').t('procedures.titles.deleteMedicationUsed'),
message: this.get('i18n').t('procedures.messages.deleteMedication'),
message: this.get('i18n').t('messages.delete_singular', { name: 'medication' }),
Copy link
Member

Choose a reason for hiding this comment

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

This code is passing a non localized string when it should be passing in a localized string or it should use the name of the item being deleted.

@@ -5,6 +5,6 @@
updateButtonText=updateButtonText }}
<div class="alert alert-danger">
<span class="glyphicon glyphicon-warning-sign"></span>
{{t 'messages.areYouSureDelete' user=model.name}}
{{t 'messages.delete' name=(concat "the user " model.name)}}
Copy link
Member

Choose a reason for hiding this comment

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

This code is passing a non localized string when it should be passing in a localized string or it should use the name of the item being deleted.

@@ -5,7 +5,7 @@
updateButtonText=updateButtonText }}
<div class="alert alert-danger">
<span class="glyphicon glyphicon-warning-sign"></span>
{{t 'visits.messages.delete'}}
{{t 'messages.delete_singular' name='visit'}}
Copy link
Member

Choose a reason for hiding this comment

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

This code is passing a non localized string when it should be passing in a localized string or it should use the name of the item being deleted.

@@ -5,6 +5,6 @@
updateButtonText=updateButtonText }}
<div class="alert alert-danger">
<span class="glyphicon glyphicon-warning-sign"></span>
{{t "procedures.messages.delete"}}
{{t "messages.delete_singular" name='procedure'}}
Copy link
Member

Choose a reason for hiding this comment

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

This code is passing a non localized string when it should be passing in a localized string or it should use the name of the item being deleted.

@@ -5,6 +5,6 @@
updateButtonText=updateButtonText }}
<div class="alert alert-danger">
<span class="glyphicon glyphicon-warning-sign"></span>
{{t "vitals.messages.delete"}}
{{t "messages.delete_plural" name='vitals'}}
Copy link
Member

Choose a reason for hiding this comment

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

This code is passing a non localized string when it should be passing in a localized string or it should use the name of the item being deleted.

@baoqchau
Copy link
Contributor Author

@jkleinsc Is this better? I put all the terms under the "models" in the translation file.

@jkleinsc
Copy link
Member

@baoqchau Those terms should be under the models section, but they need to follow the structure documented in #547. For example instead of:

models: {
  ....
  localizedStrings: {
    appointment: 'appointment'
    ....
  }
}

it should instead be:

models: {
    appointment: {
    labels: {
      ...
    },
    names: {
      singular: 'appointment'
    }
}

Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

@baoqchau looks good except for a couple of small changes I commented on. Once those are made, we should be able to merge this in.

@@ -5,6 +5,6 @@
updateButtonText=updateButtonText }}
<div class="alert alert-danger">
<span class="glyphicon glyphicon-warning-sign"></span>
{{t 'patients.messages.deletePatient' firstName=model.firstName lastName=model.lastName}}
{{t 'messages.delete' name=(concat model.firstName " " model.lastName)}}
Copy link
Member

Choose a reason for hiding this comment

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

This should user model.shortDisplayName instead of concatenating first and lastName

@@ -5,6 +5,6 @@
updateButtonText=updateButtonText }}
<div class="alert alert-danger">
<span class="glyphicon glyphicon-warning-sign"></span>
{{t 'messages.areYouSureDelete' user=model.name}}
{{t 'messages.delete' name=(concat (t 'models.user.names.singular') " " model.name)}}
Copy link
Member

Choose a reason for hiding this comment

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

Don't use concat here because the order could be different in different languages. For user I think it would be fine to have the message be:
Are you sure you wish to delete
so you can drop the

(concat (t 'models.user.names.singular') " " 

@jkleinsc jkleinsc merged commit be4cca9 into HospitalRun:master Mar 20, 2017
@billybonks
Copy link
Contributor

oh man this is awesome thank you @baoqchau

@jkleinsc jkleinsc mentioned this pull request Apr 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants