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

feat(appdistribution): add in-app-feedback support #1463

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

thatfiredev
Copy link
Member

No description provided.

Copy link

@lfkellogg lfkellogg left a comment

Choose a reason for hiding this comment

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

Nice! Left some comments.

By the way, do you intend to update the README as well? https://github.com/firebase/quickstart-android/blob/master/appdistribution/README.md

@thatfiredev
Copy link
Member Author

@lfkellogg Great callout on the README. I updated it and its screenshot. PTAL

Comment on lines +74 to +79
@Override
protected void onDestroy() {
super.onDestroy();
// Hide the notification once this activity is destroyed
mFirebaseAppDistribution.cancelFeedbackNotification();
}

Choose a reason for hiding this comment

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

You shouldn't need this anymore now that we made the change to automatically hide the notification

Copy link
Member Author

Choose a reason for hiding this comment

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

When does the automatic hide take place? I tried closing the Activity, but it didn't dismiss the notification. Is it when the app is closed?


private void askNotificationPermission() {
// This is only necessary for API level >= 33 (TIRAMISU)
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {

Choose a reason for hiding this comment

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

I don't think you want this check here anymore. Even if they are on an older device we still want to show the notification. On older devices the checkSelfPermission() call will return true.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc: @marinacoelho
The check is due to the POST_NOTIFICATIONS field which was introduced in API 33.
I have added an else branch to display the feedback notification on older devices.

Screenshot 2023-03-07 at 13 12 01

appdistribution/app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
override fun onDestroy() {
super.onDestroy()
// Hide the notification once this activity is destroyed
firebaseAppDistribution.cancelFeedbackNotification()
Copy link
Contributor

Choose a reason for hiding this comment

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

According to Lee's comment above, I think we don't need this line here as well.


private fun askNotificationPermission() {
// This is only necessary for API level >= 33 (TIRAMISU)
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
Copy link
Contributor

@marinacoelho marinacoelho Mar 7, 2023

Choose a reason for hiding this comment

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

Same as Lee's comment above. And if we absolutely need to keep this check here, what do you think of changing this function a bit to use when instead of if / else if / else ? (Or switch / case on the Java file).

Copy link
Member Author

Choose a reason for hiding this comment

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

Having a when here might work for Kotlin, since the language allows usage of when without a subject. But not sure if Java has a switch-case without a subject 🤔

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.

3 participants