Migrate over to mobx #19
Conversation
Note that this is a initial implementation, and mobx may be used inneficiently at the moment
MrCyjaneK
force-pushed
the
CW-912-cleanup1-mobx
branch
from
a7c007d to
08f4749
Compare
- Move `showExtra` parameter from form builder view model to view - Add wallet name collision check in Monero wallet info - Improve MobX state management in create wallet view - Fix screen titles not updating (or updating in wrong order) - Drop FormBuilder dependency on AbstractView (it doesn't play well when nested with mobx)
| @computed | ||
| String get screenNameOriginal => switch (createMethod) { |
There was a problem hiding this comment.
createMethod is not an observable
| @computed | |
| String get screenNameOriginal => switch (createMethod) { | |
| String get screenNameOriginal => switch (createMethod) { |
| if (coins.length == 1) { | ||
| return coins[0]; | ||
| } | ||
| return null; | ||
| }(); | ||
|
|
||
| late StringFormElement walletName = StringFormElement( | ||
| late final StringFormElement walletName = StringFormElement( |
There was a problem hiding this comment.
Minor
no need for late if it's already initialized
| @@ -1,6 +1,6 @@ | |||
| import 'package:cupcake/view_model/abstract.dart'; | |||
|
|
|||
| class InitialSetupViewModel extends ViewModel { | |||
| class InitialSetupViewModel with ViewModel { | |||
There was a problem hiding this comment.
I believe we should leave it as extends and for other view models as well, it can extend the base ViewModel and still have a mixin Store.
that way the view models would be considered of the type view model, which I guess you are using the parent class in many places
| @@ -51,12 +58,11 @@ class OpenWalletViewModel extends ViewModel { | |||
| } | |||
| } | |||
|
|
|||
| @action | |||
There was a problem hiding this comment.
not sure why those are marked with @action
| @@ -22,6 +22,7 @@ class HomeScreen extends AbstractView { | |||
|
|
|||
| @override | |||
| Widget? body(final BuildContext context) { | |||
| final _ = viewModel.varWalletSort; // work around for mobx not updating | |||
There was a problem hiding this comment.
Minor:
if it's not rebuilding with line 34, then maybe that gives us a reason why it's not ideal to have the observer on the top of the widget tree and just expect it to rebuild whenever needed
just wanted to mention that having the observer only wrapped around the needed widget would be better
| @@ -115,10 +104,9 @@ class _FormBuilderState extends State<FormBuilder> { | |||
| ), | |||
| ), | |||
| const SizedBox(height: 32), | |||
| if (MediaQuery.of(widget.scaffoldContext).viewInsets.bottom == 0) | |||
| if (MediaQuery.of(viewModel.scaffoldContext).viewInsets.bottom == 0) | |||
There was a problem hiding this comment.
I see this is the only use for scaffoldContext why not use the current context and removing the context from the view model?
| onChange: (final bool value) async { | ||
| if (value) return; | ||
| viewModel.configBiometricEnabled = false; | ||
| viewModel.config.biometricEnabled = false; |
There was a problem hiding this comment.
do we not need to call viewModel.save(); here?
| formElements: viewModel.form, | ||
| scaffoldContext: context, | ||
| isPinSet: false, | ||
| onLabelChange: (final _) {}, |
There was a problem hiding this comment.
Minor:
if we don't need it, why is it required
| } | ||
|
|
||
| @action |
There was a problem hiding this comment.
This and some others as well, I don't see a need for them to be action, maybe I am missing something, but does it modify an observable?
| @observable | ||
| int varWalletSort = CupcakeConfig.instance.walletSort; | ||
|
|
||
| @computed |
No description provided.