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

Add a minimum time criterion #4

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joeljfischer
Copy link

Fixes #2

Proposed Changes

  • Add an optional minimum time criterion between the first active positive event and the current time

* Add an optional minimum time criterion between the first active positive event and the current time
@joeljfischer joeljfischer force-pushed the feature/issue-2-minimum-time-before-request-review branch from 2c184f8 to 3ffc597 Compare March 15, 2024 00:20
Copy link
Member

@Jeehut Jeehut left a comment

Choose a reason for hiding this comment

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

Thank you for suggesting this feature and for the example implementation. 🙏

I do understand your point that in some apps you might not want to request a review on the first day. I do not think that this is something that should be enabled by default or even recommended in the library though – from my personal experience and what I read elsewhere it's actually best practice to already ask for review on first app use given that some positive actions occurred. But we could certainly cover your use case to add some more flexibility to the library.

To keep the library and docs simple, I would actually prefer to give this option its own property though. I'm also not sure about the naming being minimumTimeBeforeRequest – it's not very clear to me what it refers to. I initially thought what you're asking for is a delay right after requestReview is called until the prompt is presented. So I would prefer a name along the lines of avoidRequestsOnFirstDay to be more clear. Maybe we don't even need any other options than to provide a Bool?

Another question is: Should the delay really relate to the first event in the list? We could easily add another storage for the first time a call was made, then it could be related to the first day app is used.

@joeljfischer
Copy link
Author

Regarding the time frame, I would prefer something more flexible like what I currently have implemented. In my app I have a delay of a week, for example. I understand the current naming being unclear. If you wanted to go the direction of using a Bool, that's fine, I already have a fork that I'm using.

As far as the first event in the list vs. the first app open, I think the first event in the list makes more sense. What I was trying to avoid was a situation where the user opens the app, then doesn't use it for a month, then opens it again and starts using it. I'd prefer to see the later app open as the new "first time it's used".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] Minimum Time Before Requesting Review
2 participants