-
Notifications
You must be signed in to change notification settings - Fork 207
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
Patron - Add icons #953
Patron - Add icons #953
Changes from 10 commits
74aa4ba
601dfc1
b32bd4f
4d38c45
054e324
e50e09b
1dcd6f1
d0f7b15
15e74b4
46fce64
f6f5a8a
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 |
---|---|---|
|
@@ -50,6 +50,7 @@ class OnboardingUpgradeFeaturesViewModel @Inject constructor( | |
val state: StateFlow<OnboardingUpgradeFeaturesState> = _state | ||
|
||
private val source = savedStateHandle.get<OnboardingUpgradeSource>("source") | ||
private val showPatronOnly = savedStateHandle.get<Boolean>("show_patron_only") | ||
Comment on lines
52
to
+53
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. Instead of hardcoding these, I think it would be good to reuse the variables from 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. Already tried but there's a cyclic dependency between the account and profile modules 😞 |
||
|
||
init { | ||
if (BuildConfig.ADD_PATRON_ENABLED) { | ||
|
@@ -89,7 +90,7 @@ class OnboardingUpgradeFeaturesViewModel @Inject constructor( | |
val lastSelectedTier = settings.getLastSelectedSubscriptionTier().takeIf { source in listOf(OnboardingUpgradeSource.LOGIN, OnboardingUpgradeSource.PROFILE) } | ||
val lastSelectedFrequency = settings.getLastSelectedSubscriptionFrequency().takeIf { source in listOf(OnboardingUpgradeSource.LOGIN, OnboardingUpgradeSource.PROFILE) } | ||
|
||
val showPatronOnly = source == OnboardingUpgradeSource.ACCOUNT_DETAILS | ||
val showPatronOnly = source == OnboardingUpgradeSource.ACCOUNT_DETAILS || showPatronOnly == true | ||
val updatedSubscriptions = | ||
if (showPatronOnly) { | ||
subscriptions.filter { it.tier == Subscription.SubscriptionTier.PATRON } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,12 @@ | ||
<vector android:height="24dp" android:viewportHeight="30" | ||
android:viewportWidth="30" android:width="24dp" xmlns:android="http://schemas.android.com/apk/res/android"> | ||
<path android:fillColor="#03A9F4" android:pathData="M15,28.75C7.4061,28.75 1.25,22.5939 1.25,15C1.25,7.4061 7.4061,1.25 15,1.25C22.5939,1.25 28.75,7.4061 28.75,15C28.75,22.5939 22.5939,28.75 15,28.75Z"/> | ||
<path android:fillColor="#ffffff" android:pathData="M19.0458,10.4426C19.4917,9.9156 20.2804,9.8499 20.8074,10.2958C21.3344,10.7417 21.4002,11.5304 20.9542,12.0575L13.2017,21.2195L8.4911,16.5089C8.0029,16.0208 8.0029,15.2293 8.4911,14.7411C8.9793,14.253 9.7707,14.253 10.2589,14.7411L13.0483,17.5305L19.0458,10.4426Z"/> | ||
<vector xmlns:android="http://schemas.android.com/apk/res/android" | ||
android:width="40dp" | ||
android:height="40dp" | ||
android:viewportWidth="40" | ||
android:viewportHeight="40"> | ||
<path | ||
android:pathData="M20,33.75C12.406,33.75 6.25,27.594 6.25,20C6.25,12.406 12.406,6.25 20,6.25C27.594,6.25 33.75,12.406 33.75,20C33.75,27.594 27.594,33.75 20,33.75Z" | ||
android:fillColor="#03A9F4"/> | ||
<path | ||
android:pathData="M24.046,15.443C24.492,14.916 25.28,14.85 25.807,15.296C26.334,15.742 26.4,16.53 25.954,17.058L18.202,26.219L13.491,21.509C13.003,21.021 13.003,20.229 13.491,19.741C13.979,19.253 14.771,19.253 15.259,19.741L18.048,22.531L24.046,15.443Z" | ||
android:fillColor="#ffffff"/> | ||
</vector> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
<vector xmlns:android="http://schemas.android.com/apk/res/android" | ||
android:width="40dp" | ||
android:height="40dp" | ||
android:viewportWidth="40" | ||
android:viewportHeight="40"> | ||
<path | ||
android:pathData="M20.117,20.117m-13.749,-0.118a13.75,13.75 124.03,1 1,27.499 0.235a13.75,13.75 124.03,1 1,-27.499 -0.235" | ||
android:fillColor="#6046F5"/> | ||
<path | ||
android:pathData="M16.75,18.031V17.5C16.75,15.705 18.205,14.25 20,14.25C21.795,14.25 23.25,15.705 23.25,17.5V18.031C23.681,18.142 24,18.534 24,19V24C24,24.552 23.552,25 23,25H17C16.448,25 16,24.552 16,24V19C16,18.534 16.319,18.142 16.75,18.031ZM18.25,17.5C18.25,16.534 19.034,15.75 20,15.75C20.966,15.75 21.75,16.534 21.75,17.5V18H18.25V17.5Z" | ||
android:fillColor="#ffffff" | ||
android:fillType="evenOdd"/> | ||
</vector> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
<vector xmlns:android="http://schemas.android.com/apk/res/android" | ||
xmlns:aapt="http://schemas.android.com/aapt" | ||
android:width="40dp" | ||
android:height="40dp" | ||
android:viewportWidth="40" | ||
android:viewportHeight="40"> | ||
<path | ||
android:pathData="M20.117,20.117m-13.749,-0.118a13.75,13.75 124.03,1 1,27.499 0.235a13.75,13.75 124.03,1 1,-27.499 -0.235"> | ||
<aapt:attr name="android:fillColor"> | ||
<gradient | ||
android:startX="8.941" | ||
android:startY="6.271" | ||
android:endX="33.794" | ||
android:endY="8.635" | ||
android:type="linear"> | ||
<item android:offset="0" android:color="#FFFED745"/> | ||
<item android:offset="1" android:color="#FFFEB525"/> | ||
</gradient> | ||
</aapt:attr> | ||
</path> | ||
<path | ||
android:pathData="M16.75,18.031V17.5C16.75,15.705 18.205,14.25 20,14.25C21.795,14.25 23.25,15.705 23.25,17.5V18.031C23.681,18.142 24,18.534 24,19V24C24,24.552 23.552,25 23,25H17C16.448,25 16,24.552 16,24V19C16,18.534 16.319,18.142 16.75,18.031ZM18.25,17.5C18.25,16.534 19.034,15.75 20,15.75C20.966,15.75 21.75,16.534 21.75,17.5V18H18.25V17.5Z" | ||
android:fillColor="#ffffff" | ||
android:fillType="evenOdd"/> | ||
</vector> |
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'm passing
showPatronOnly
boolean as another argument in the upsell flow so that I can access it inOnboardingUpgradeFeaturesViewModel
init and show only the Patron plans if the key is set.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.
Am I right that we don't actually pass this argument when we navigate, but instead we're just using the default value (and we have some logic to intelligently set the default)? If so, that feels a bit confusing to me.
I've definitely run into this issue of how best to get some data from the compose layer to the view model, and it doesn't seem like there's a clear best approach. I wonder if it would be worth directly passing the
flow
to the view model here in thecomposable
's lambda parameter with something like this (untested) code:What you have is working fine though, so feel free to leave it as-is if you prefer. I'm still not sure how best to handle this tbh.
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.
Thinking about this some more, It seems like my suggestion would break if multiple views were using this view model (probably not an issue in this case, but as a general matter could be a problem) since currently it is not possible to scope a view model to a particular composable. 🤔
I'm thinking about this more because I have the same issue, but I can't use navigation arguments for my use case really (well, I could, but it would be pretty hacky and hard to understand). Looks like this might be a possibility as well, but I think it's too complicated for me for now. Just sharing this as additional info, it's probably not really applicable to what you're doing here.
While I'm saving links, here is a relevant discussion issue about not currently being able to use assisted injection with hilt.
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.
That's true. Passing objects via routes from Compose layer to view model is currently not supported. There are hacks to do so using custom nav type and json string <-> object conversion but passing complex data is considered an anti-pattern. I also considered using assisted variables in
HiltViewModel
but as you pointed it is currently not supported for Compose.I understand what you're saying but I wasn't able to find a simpler solution. The approach I took in this PR follows official guidelines to pass nav args similar to how they are passed in "implicit" deep links. Default values are set as arguments for a start destination, the navigation component parses them based on the deep-link-like route that we provide and allows us to access them using
saveStateHandle
in the view model.It is also explained here. Since I require simple boolean and string sources in the view model, sending them using routes fits my case.
To make it more natural so that the route looks closer to a deep-link-like structure, I made
showPatronOnly
an optional param and added a comment linking the documentation to help understand it in f6f5a8aMaybe I should also convert the source string to an optional one. 🤔
The recommended approach to getting complex data in view models is getting them using the data layer like through a repository. Lmk your use case though I'm not sure if there are good options currently.