Patron - Add icons #953
Patron - Add icons #953
Conversation
| // The route variable should only be used to navigate to the PlusUpgrade screens | ||
| // when they are the startDestination. In all other cases, use the routeWithSource function. | ||
| const val route = "$routeBase/{$sourceArgumentKey}" | ||
| const val route = "$routeBase/{$sourceArgumentKey}/{$showPatronOnlyArgumentKey}" |
There was a problem hiding this comment.
I'm passing showPatronOnly boolean as another argument in the upsell flow so that I can access it in OnboardingUpgradeFeaturesViewModel init and show only the Patron plans if the key is set.
There was a problem hiding this comment.
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 the composable's lambda parameter with something like this (untested) code:
val viewModel = hiltViewModel<OnboardingUpgradeFeaturesViewModel>()
LaunchedEffect(flow) {
viewModel.setFlow(flow)
}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.
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.
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.
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.
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 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.
The Navigation component attempts to parse the placeholder values into appropriate types by matching placeholder names to the defined arguments that are defined for the deep link destination.
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 f6f5a8a
New: "$routeBase/{$sourceArgumentKey}?{$showPatronOnlyArgumentKey}={$showPatronOnlyArgumentKey}"
Vs
Old: "$routeBase/{$sourceArgumentKey}/{$showPatronOnlyArgumentKey}"
Maybe I should also convert the source string to an optional one. 🤔
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).
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.
| private val source = savedStateHandle.get<OnboardingUpgradeSource>("source") | ||
| private val showPatronOnly = savedStateHandle.get<Boolean>("show_patron_only") |
There was a problem hiding this comment.
Instead of hardcoding these, I think it would be good to reuse the variables from OnboardingNavRoute. In addition to ensuring they stay consistent, this also makes it easier to use code navigation to see where this argument is being set and used.
There was a problem hiding this comment.
Already tried but there's a cyclic dependency between the account and profile modules 😞
| // The route variable should only be used to navigate to the PlusUpgrade screens | ||
| // when they are the startDestination. In all other cases, use the routeWithSource function. | ||
| const val route = "$routeBase/{$sourceArgumentKey}" | ||
| const val route = "$routeBase/{$sourceArgumentKey}/{$showPatronOnlyArgumentKey}" |
There was a problem hiding this comment.
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 the composable's lambda parameter with something like this (untested) code:
val viewModel = hiltViewModel<OnboardingUpgradeFeaturesViewModel>()
LaunchedEffect(flow) {
viewModel.setFlow(flow)
}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.
|
@mchowning I'm merging this PR, feel free to suggest any changes and I'll take it up in another PR. |
Description
This adds icons Patron icons and starts upsell flow to show only Patron plans.
Testing Instructions
Test.1 Feature Flag disabled
Test.2 Feature Flag enabled - Plus only
Test.3 Feature Flag enabled - Patron
Screenshots or Screencast
Screen_recording_20230511_182351.mp4
Checklist
modules/services/localization/src/main/res/values/strings.xmlI have tested any UI changes...