Skip to content
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

Modiy refer banner #17598

Merged
merged 8 commits into from May 20, 2021
Merged

Modiy refer banner #17598

merged 8 commits into from May 20, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented May 19, 2021

Description:

fixes #17571

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@flamisz flamisz self-assigned this May 19, 2021
@flamisz flamisz added this to the 4.3.1 milestone May 19, 2021
@flamisz flamisz added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label May 19, 2021
Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

looks good @flamisz 👍 Left a comment and wondering if we can directly close the popup once a share link was clicked and then we never show it again. Assuming most people would only share through one network anyway.

I haven't looked yet is it already implemented to not show the banner again once clicked on a social network? Could maybe show in the thank you message otherwise "Thank you... we won't show this banner again" or something?

@@ -135,7 +139,12 @@ public function showReferBanner()
$nextReminderDate = $referReminder->getUserOption();

if ($nextReminderDate === false) {
return true;
$nextReminder = Date::now()->getStartOfDay()->addDay(6 * 30)->toString('Y-m-d');
Copy link
Member

Choose a reason for hiding this comment

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

@flamisz I believe the "review reminder" is showing every 90 days so there's a risk both review and refer banners and shown at same point.

Maybe after 6 months (180 days) we might show both reminders approx on a similar day? The 90 days in a review reminder is sent from plugins/Feedback/angularjs/feedback-popup/feedback-popup.controller.js. I now wonder if we maybe could add 135 days to the next "review reminder" date? This way it be 45 days before the popup would be shown. If no review reminder date is set yet, then we would maybe need to set the review reminder to be in 90 days and the refer reminder to be in 135 days?

Bit complicated just wanting to avoid them being shown to close of each other :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I check in the code, at line 111:

if ($this->getShouldPromptForFeedback()) {
...

This way we can't show it at the same time.
But to make sure they don't come up close to each other (next day, etc), make sense. I'll check your recommendation and add it.

@flamisz
Copy link
Contributor Author

flamisz commented May 20, 2021

looks good @flamisz 👍 Left a comment and wondering if we can directly close the popup once a share link was clicked and then we never show it again. Assuming most people would only share through one network anyway.

I haven't looked yet is it already implemented to not show the banner again once clicked on a social network? Could maybe show in the thank you message otherwise "Thank you... we won't show this banner again" or something?

We can do that, yes. I think originally we tried something like this but modified thinking about someone who wants to share on multiple platforms.
In this version, because we have the 2 buttons after the user shares it, I don't close it, just show the thanks message and the user can decide which button to use closing it.
But I like the idea to close it, don't show it again after a user shares even on one platform. They can share it without this modal if they want later too.

So I will modify it to close the modal, save it to not show it again, and show the thanks message with the recommended text.

@flamisz flamisz marked this pull request as draft May 20, 2021 03:32
@flamisz flamisz marked this pull request as ready for review May 20, 2021 04:42
@flamisz flamisz added the Needs Review PRs that need a code review label May 20, 2021
@tsteur
Copy link
Member

tsteur commented May 20, 2021

Cheers @flamisz 💯

@tsteur tsteur merged commit 65d9819 into 4.x-dev May 20, 2021
@tsteur tsteur deleted the 17571-modify-refer-banner branch May 20, 2021 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

annoying banner (the 'refer matomo' banner at the bottom of the page)
2 participants