Issue #9208: Adds in-product prompt to homescreen #9836
Conversation
1b23f85
to
4eb7d36
Compare
Codecov Report
@@ Coverage Diff @@
## master #9836 +/- ##
============================================
- Coverage 19.25% 19.07% -0.18%
Complexity 533 533
============================================
Files 340 343 +3
Lines 13806 13942 +136
Branches 1849 1869 +20
============================================
+ Hits 2658 2660 +2
- Misses 10909 11042 +133
- Partials 239 240 +1
Continue to review full report at Codecov.
|
Request for data collection review formAll questions are mandatory. You must receive review from a data steward peer on your responses to these questions before shipping new data collection.
|
BUTTON | ||
} | ||
|
||
open class Tip( |
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.
Nit: Since we're adding the abstraction of a ButtonType. Could we move some of the data that is associated with that type to the type itself?
sealed class Type {
data class Button(val title: String, action: () -> Unit)
}
@StringRes val identifier: Int, | ||
@DrawableRes val icon: Int = -1, | ||
@StringRes val title: Int, | ||
@StringRes val description: Int, |
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.
Could we have "learn more" be a link inside of the description versus having a separate section? This would allow us to have it inline like the mocks
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.
The reason I didn't do this is for accessibility purposes. I remember a while back that we had to move our "learn more" links into separate views so they would be separately selectable for screen readers. See the private browsing "myths" and the private browsing "allow search suggestions" hint.
@StringRes val identifier: Int, | ||
@DrawableRes val icon: Int = -1, | ||
@StringRes val title: Int, | ||
@StringRes val description: Int, |
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.
what are the thoughts behind making this resource values versus the actual string / drawables?
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.
Really happy with doing either here, do you think doing actual strings/drawables is cleaner? :)
bd7fd65
to
6be4083
Compare
b1a2587
to
bf39e9c
Compare
@@ -137,6 +137,12 @@ class Settings private constructor( | |||
default = true | |||
) | |||
|
|||
// If any of the prefs have been modified, quit displaying the fenix moved tip | |||
fun shouldDisplayFenixMovingTip(): Boolean = | |||
preferences.getBoolean(appContext.getString(R.string.pref_key_migrating_from_fenix_nightly_tip), true) && |
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.
should this be || instead of &&?
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.
I don't think so. Each of these default to "true" and each user will get served one of these fenix moving tips. If any of them become "false" we no longer want to display the tip.
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.
If it was an OR here, then they'd always get the tip and have no way to set the other pref keys to false :)
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.
data-review only
Data Review Form (to be filled by Data Stewards)
- Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way?
Yes, tip behavior and user interaction with it is documented in metrics.yaml.
- Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available.
Yes, in Fenix data controls
- If the request is for permanent data collection, is there someone who will monitor the data over time?
Expiry set for 09/2020
- Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?
Type 2, when tips are shown and interacted with by the user.
- Is the data collection request for default-on or default-off?
Default on
- Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?
No, interaction only
- Is the data collection covered by the existing Firefox privacy notice?
Yes, feature interaction
- Does there need to be a check-in in the future to determine whether to renew the data?
no, has expiry
- Does the data collection use a third-party collection tool?
No
Pull Request checklist
After merge
To download an APK when reviewing a PR: