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

Rewarded ads rrme #3542

Merged
merged 6 commits into from
Aug 16, 2024
Merged

Conversation

mhkawano
Copy link
Contributor

@mhkawano mhkawano commented Aug 15, 2024

Enable rewarded ads for RRME.

Demo: b/358588957

@mhkawano mhkawano closed this Aug 16, 2024
@mhkawano mhkawano reopened this Aug 16, 2024
@mhkawano mhkawano marked this pull request as ready for review August 16, 2024 00:44
}).start();
} else if (this.intervention.type == InterventionType.TYPE_REWARDED_AD) {
return new AudienceActionLocalFlow(this.deps_, {
action: this.intervention.type,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is minor but can we keep the argument order in sync with the above?

@@ -599,6 +611,7 @@ export class AudienceActionLocalFlow implements AudienceActionFlow {
AnalyticsEvent.ACTION_REWARDED_AD_CLOSE,
/* isFromUserAction */ true
);
this.rewardedResult(true, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add parameter name comments so that it's understanable without having to check the function definition.

See go/tsjs-style#comments-when-calling-a-function
“Parameter name” comments should be used whenever the method name and parameter value do not sufficiently convey the meaning of the parameter.

@@ -558,9 +561,12 @@ export class AudienceActionLocalFlow implements AudienceActionFlow {
AnalyticsEvent.ACTION_REWARDED_AD_CLOSE_AD,
/* isFromUserAction */ true
);
this.rewardedResult(true, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as below

this.closeOptInPrompt_();
}
}

private rewardedResult(
Copy link
Contributor

Choose a reason for hiding this comment

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

The function name doesn't seem to convey what exactly it does. Can you rename it to be more specific on what it does?

@mhkawano mhkawano merged commit efecc8d into subscriptions-project:main Aug 16, 2024
7 checks passed
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.

2 participants