-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Issue/11799 show announcement on app upgrade #11804
Changes from all commits
f4d1fb3
06492c9
0deadf4
08be308
f908cf4
16165c8
06052ec
9f50e49
1f42fec
1d7fd07
bf7db04
abae59f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package org.wordpress.android.util | ||
|
||
import org.wordpress.android.BuildConfig | ||
import javax.inject.Inject | ||
|
||
class BuildConfigWrapper @Inject constructor() { | ||
fun getAppVersionCode(): Int { | ||
return BuildConfig.VERSION_CODE | ||
} | ||
|
||
fun isFeatureAnnouncementEnabled(): Boolean { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we are introducing the wrapper, wdyt if we use it in AppSettingsFragment.java Line 172 also? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the check to AppSettingsFragment in upcoming PR :) |
||
return BuildConfig.FEATURE_ANNOUNCEMENT_AVAILABLE | ||
} | ||
} |
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.
Not sure it's worth nor how much work it can be (so feel free to just say you will never consider doing 😊), but since we are possibly considering expanding the announcements one day to include some of the previous one and to avoid this (sometimes tricky) nullable, maybe we could consider to use a list of FeatureAnnouncement (eventually ordered by app version code or being empty)...wdyt?
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 added the
getFeatureAnnouncements(): List<FeatureAnnouncement>
method in upcoming PR, but it's not really solving the nullability of the announcement, it just moves the check upstream.