Hotfix/api feedback form #1573
Hotfix/api feedback form #1573
Conversation
| @@ -9,6 +9,7 @@ AutoForm.hooks({ | |||
| }, | |||
| onSuccess () { | |||
| sAlert.success('Thank you! Your feedback has been successfully sent.'); | |||
There was a problem hiding this comment.
Add i18n token here. Use the following pattern, for clarity and consistency:
// Get feedback success message translation
const message = TAPi18n.__('feedbackForm_successMessage');
// Alert user of success
sAlert.success(message);
|
@NNN Re-assigning Brylie for the PR review, because we would need to get this forward and there has been no visible activity in discussion. |
| type="insert" | ||
| omitFields="authorId, createdAt, apiBackendId" | ||
| }} | ||
| <h4 class="modal-title" id="feedbackFormModalLabel">{{_ "feedback_feedbackForm_title"}}</h4> |
There was a problem hiding this comment.
Break this into nested lines, for readability and to shorten the line length:
<h4 class="modal-title" id="feedbackFormModalLabel">
{{_ "feedback_feedbackForm_title"}}
</h4>
|
This looks good. There is just one missing i18n string and some markup lint. I can merge this today, if you can make the suggested changes in the next 30 minutes. |
# Conflicts: # feedback/client/form/autoform.js
|
Suggested changes are done. Please review |
| @@ -8,7 +8,7 @@ AutoForm.hooks({ | |||
| }, | |||
| }, | |||
| onSuccess () { | |||
| sAlert.success('Thank you! Your feedback has been successfully sent.'); | |||
| sAlert.success(TAPi18n.__('feedbackForm_successMessage')); | |||
There was a problem hiding this comment.
While this line is compact, it does not follow convention for other translated user alerts.
Refactor the code to follow this pattern:
// Get the success message translation
const message = TAPi18n.__('feedbackForm_successMessage');
// Alert the user of success
sAlert.success(message);The reason is to make lines of code easier to read:
- shorter lines
sAlert.success(message)is very close to English 'alert success message'
Search for other uses of sAlert in our project, to get a sense of how it is typically used.
There was a problem hiding this comment.
You've commented on an outdated diff. That file has already been changed.
Closes #1525.