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

Making it easier to override default IMvxViewModelTypeFinder implementation #2498

Merged
merged 6 commits into from
Jan 10, 2018

Conversation

nickrandolph
Copy link
Contributor

@nickrandolph nickrandolph commented Dec 30, 2017

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Feature

⤵️ What is the current behavior?

Currently in order to override the implementation of IMvxViewModelTypeFinder you have to override the entire InitializeViewModelTypeFinder method in each setup for your solution

🆕 What is the new behavior (if this is a feature change)?

Can now simply override CreateViewModelByNameLookup, CreateViewModelByNameRegistry or RegisterViewTypeFinder to change the lookup, registry or finder

💥 Does this PR introduce a breaking change?

No

🐛 Recommendations for testing

📝 Links to relevant issues/docs

N/A

🤔 Checklist before submitting

  • All projects build
  • Follows style guide lines (code style guide)
  • Relevant documentation was updated (docs style guide)
  • Nuspec files were updated (when applicable)
  • [ X ] Rebased onto current develop

@martijn00
Copy link
Contributor

I'm not sure this is a good idea, every sort of view in MvvmCross is Mvx based. This is so we can internally make changes to fix bugs. If we are supporting non mvx views it might even get harder for us to keep up to date with external changes.

What's wrong with leaving it as is?

@nickrandolph
Copy link
Contributor Author

There's just too much friction involved in getting started with MVX with Forms.
I submitted this PR mainly to get the conversation started on this - MVX is currently a chore to work with and we need to reduce the drag on developers wanting to use it.
If there's resistance to removing the dependency on Mvx based application and pages I can revert those changes and just leave the changes to the Setup that allows for it to be overridden easier. Let me know what you think

@nmilcoff
Copy link
Contributor

nmilcoff commented Jan 2, 2018

If anything is difficult to override or if anything fails, we should just fix it / improve it / document it.

But other than that I think MvvmCross has always been more of a full framework than something you can just plug in in an app. I'd say that discussion should be moved into a separate issue.

@nickrandolph nickrandolph changed the title Removing dependencies on MvxFormsApplication and IMvxView Making it easier to override default IMvxViewModelTypeFinder implementation Jan 6, 2018
@nickrandolph
Copy link
Contributor Author

@martijn00 @nmilcoff I've reverted the changes concerning MvxApplication and MvxContentPage, leaving on the change that makes it easier to override the default finder.

nmilcoff
nmilcoff previously approved these changes Jan 8, 2018
@nmilcoff nmilcoff added this to the 6.0.0 milestone Jan 8, 2018
martijn00
martijn00 previously approved these changes Jan 10, 2018
@martijn00 martijn00 merged commit d540343 into MvvmCross:develop Jan 10, 2018
@nickrandolph nickrandolph deleted the nick/mvx-forms-refactor branch January 13, 2018 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants