-
Notifications
You must be signed in to change notification settings - Fork 624
Add info text argument to startFeedback #3971
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
Conversation
91b3148
to
734d6ae
Compare
public static final String SCREENSHOT_FILENAME_EXTRA_KEY = | ||
"com.google.firebase.appdistribution.FeedbackActivity.SCREENSHOT_FILE_NAME"; | ||
|
||
private FeedbackSender feedbackSender; | ||
private String releaseName; | ||
private CharSequence infoText; |
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.
Just curious, how do you choose CharSequence
instead of a plain ole' String
?
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.
Because that's what Context.getText()
returns: https://developer.android.com/reference/android/content/Context#getText(int)
Context.getString()
returns a String but it does not retain any html/styles, which we need.
public void startFeedback() { | ||
startFeedback(Executors.newSingleThreadExecutor()); | ||
public void startFeedback(int infoTextResourceId) { | ||
startFeedbackWithInfoText(firebaseApp.getApplicationContext().getText(infoTextResourceId)); |
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.
Can you chain to startFeedback(String)
instead of directly to startFeedbackWithInfoText
, so the implementations are less likely to diverge?
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.
Actually I'm realizing our API should have startFeedback(CharSequence)
, so yes.
I was trying to model our API after TextView.setText, which accepts an int or a CharSequence. I thought it was int or String.
Coverage Report 1Affected Products
Test Logs
Notes |
@@ -4,6 +4,7 @@ | |||
xmlns:tools="http://schemas.android.com/tools" | |||
android:layout_width="match_parent" | |||
android:layout_height="match_parent" | |||
android:padding="24dp" |
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.
Is this just generated XML changes? I kind of just skimmed this.
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.
These add the info text to our proof-of-concept UX.
The public api surface has changed for the subproject firebase-appdistribution-api: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
The public api surface has changed for the subproject firebase-appdistribution-api: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
Size Report 1Affected Products
Test Logs
Notes |
The public api surface has changed for the subproject firebase-appdistribution-api: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
The public api surface has changed for the subproject firebase-appdistribution-api: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
/test smoke-tests |
Based on go/iaf-sdk-api-proposal
Tested in test app:
