-
Notifications
You must be signed in to change notification settings - Fork 21
PG-4653 CustomAlert tooltips #230
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
Conversation
AltamashShaikh
left a comment
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.
@lachiebol Did you forget to build the vue files and commit them ?
@AltamashShaikh Doh! Should be there now |
AltamashShaikh
left a comment
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.
Left some comments around translation reuse
@AltamashShaikh Thank you for educating me :) Could you please check again? The only difference is that General_LearnMore is not capitalised, I thought about doing a .toUpperCase, but have a feeling that might not work well with certain translations |
vue/src/EditAlert/EditAlert.vue
Outdated
| }, | ||
| getDeliveryMediumInlineTooltip(): string { | ||
| const link = translate( | ||
| 'General_LearnMore', |
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.
@lachiebol If we need it to be capitalized, we can add the Learnmore in the plugin translation file, like CustomAlerts_LearnMore you can check with Stan.
vue/src/EditAlert/EditAlert.vue
Outdated
| externalLink('https://matomo.org/faq/general/create-and-manage-custom-alerts/'), | ||
| '</a>', | ||
| ); | ||
| return `${translate('CustomAlerts_CreateTooltip')} ${link}`; |
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.
Do we need a fulltstop at the end ?
AltamashShaikh
left a comment
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.
Left few minor comments, rest looks good to me 👍
|
@AltamashShaikh Just changed it to be capitalised |
Description
Added tooltips in add/edit alerts, manage alerts & in the inline text for the delivery method field
Issue No
Steps to Replicate the Issue
Checklist