Skip to content
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

[WIP] Japan example app #108

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Open

[WIP] Japan example app #108

wants to merge 17 commits into from

Conversation

gigiyy
Copy link
Contributor

@gigiyy gigiyy commented Oct 4, 2018

No description provided.

btLimit.isChecked = true
btMarket.isChecked = false
btMarket.setOnClickListener {
btLimit.isChecked = false
Copy link

@falnatsheh falnatsheh Oct 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts on having btLimit and btMarket boolean in the ViewModel vs here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they are actually the button itself, here we're make it selected or not. however it can be made more concise or nature if keep the status in viewModel and linked them together. I'll try

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using the model turns out a better solution. I've got a OrderType in my OrderInfo already.

fun increasePrice() {
val value = orderForm.value
orderForm.value = value?.apply {
if (orderInfo.limitPrice < symbol.priceUpperLimit) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Swift you could remove orderForm.value = value?.apply { and replace it with orderForm?.value, like the following:

if (orderInfo?.limitPrice < symbol.priceUpperLimit) {
         orderInfo = orderInfo.copy(limitPrice = orderInfo.limitPrice + 1)
 }

Can you do that in Kotlin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. they can be inlined. updated similar places.

}

fun init(symbol: String?) {
// TODO need a cleaner way to initialize the OrderForm

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!this::orderForm.isInitialized is needed? since this is the init of the class I assume this::orderForm.isInitialized will return false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when screen rotates, Fragment will call init again. however we don't need to init the ViewModel since we should reuse the model in this case. I'm still looking if there is a way to call this init only once and when it's needed. (need to dig into how Fragment works for this)

Copy link

@falnatsheh falnatsheh Oct 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When configuration changes the fragment is destroyed and then recreated by the fragment manager. However the ViewModel survives configuration changes so when the activity is recreated and you call the ViewModelProviders here you actually get the same instance. https://developer.android.com/topic/libraries/architecture/viewmodel#lifecycle

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the view model factory works well. removed the init method now.

viewModel = ViewModelProviders.of(activity!!).get(OrderInputViewModel::class.java)
// TODO: Use the ViewModel
arguments?.getString("symbol")?.let {
viewModel.init(it)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To init a ViewModel you need to create custom factory and use it with ViewModelProviders: https://android.jlelse.eu/android-viewmodel-with-custom-arguments-d0ff0fba29e1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants