-
Notifications
You must be signed in to change notification settings - Fork 102
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
Confirm dialog modal implementation #763
Confirm dialog modal implementation #763
Conversation
@jniles could you review this PR |
@mbayopanda, the text "Veuillez saisir 'Le nom du project' pour confirmer" makes me think that I should type I would either remove the quotes (Veuillez saisir le nom du project pour confirmer) or actually template in the project name (Veuillez saisir "Project Pax Clinique" pour confirmer). Does that make sense? |
@jniles you're right. |
728db89
to
5423dc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything LGTM. Happy to merge this.
It's your choice if you want to work on the $translate
parameters now or put if off for later. If later, just say so and I'll merge this.
"PLEASE_TYPE_TEXT" : "Please type in the %ELEMENT% name to confirm", | ||
"CANNOT_UNDONE_ACTION" : "This action CANNOT BE REVERSED, this action will be permanent. You must be sure that you want to do this action", | ||
"NO_CORRESPONDANCY" : "No correspondancy found for ", | ||
"PLEASE_TYPE_TEXT" : "Please type %ELEMENT% to confirm", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reference, angular-translate provides variable substitution so that you do not have to do this manually. Documentation. An interesting PR related to this.
Since we've been using this style long term, I don't mind merging this PR. But if possible, it would be nice to move to angular-translate
's way of putting parameters into our translations. I made an issue about it a long time ago: #408.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool 👍
|
||
Modal.openConfirmDeletion(request) | ||
Modal.openConfirmDialog(request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirm Dialog is much better name 👍.
var request = { pattern: pattern, elementName: 'document'}; | ||
var request = { | ||
pattern: pattern, | ||
patternName: 'FORM.PATTERNS.DOCUMENT_NAME' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
patternName
is a much better name 👍
@@ -170,10 +170,10 @@ function EnterpriseController($translate, Enterprises, Currencies, Accounts, uti | |||
function deleteProject(id, pattern) { | |||
var params = { | |||
pattern: pattern, | |||
elementName: $translate.instant('FORM.LABELS.PROJECT') | |||
patternName: 'FORM.PATTERNS.PROJECT_NAME' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad the $translate
call has finally been moved into the modal. This is a nice improvement.
"PLEASE_TYPE_TEXT" : "Please type in the %ELEMENT% name to confirm", | ||
"CANNOT_UNDONE_ACTION" : "This action CANNOT BE REVERSED, this action will be permanent. You must be sure that you want to do this action", | ||
"NO_CORRESPONDANCY" : "No correspondancy found for ", | ||
"PLEASE_TYPE_TEXT" : "Please type {{value}} to confirm", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
This PR :
confirmDeletion
modal and replace it with theconfirmDialog
modalclose #759