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
Ask users to leave a review for Matomo #14432
Conversation
Sample screenshot below. @Findus23 Thomas has suggested that you may be able to help with the logos for the review sites - the Product Hunt one is OK as they provide official branding resources, but the others I've just taken off the internet. They need transparent backgrounds and we may need to play around with the size a bit as well, particularly for SaasWorthy. |
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.
Hi @katebutler,
Could you please update the images with the SVGs I sent you?
Thanks
@katebutler can you check @Findus23 comment from 13 days ago? |
# Conflicts: # plugins/CustomDimensions
Have replaced the icons with the SVGs. Has caused quite a few UI tests to fail because of the popup showing up over the top of the expected dashboard. My own test that the popup doesn't show up when it's not expected is also failing so hopefully once I fix that it will sort the others out. |
Ready for review/merge |
plugins/Feedback/Controller.php
Outdated
{ | ||
$nextReminder = Common::getRequestVar('nextReminder'); | ||
// -1 means "never remind me again", otherwise add the interval onto today's date | ||
if ((int)$nextReminder !== -1) { |
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.
maybe could use a const named appropriately and remove the comment?
<div ng-include="'plugins/Feedback/angularjs/feedback-popup/review-links.directive.html'"></div> | ||
|
||
<div class="footer"> | ||
<input type="button" value="Remind me later" role="yes"/> |
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.
this string is not translated yet (line below as well)
*/ | ||
public function updateFeedbackReminderDate() | ||
{ | ||
$nextReminder = Common::getRequestVar('nextReminder'); |
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.
maybe add auth check, that user is not anonymous?
@katebutler noticed tests are failing with:
|
PR looks good otherwise, looking forward to merging this 👍 |
|
Fixes #14086