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

[Feature] SE View Only #168

Closed
leonardo2204 opened this issue Oct 18, 2016 · 12 comments
Closed

[Feature] SE View Only #168

leonardo2204 opened this issue Oct 18, 2016 · 12 comments
Milestone

Comments

@leonardo2204
Copy link
Contributor

leonardo2204 commented Oct 18, 2016

What do you think about having a SE(send-error) View/Presenter only ?
That would be useful on Form Scenarios, like logging in, saving data etc.

I'm already implementing it on my app, if you think that would be cool, I can drop a PR !

@leonardo2204 leonardo2204 changed the title [Feature] LE View Only [Feature] SE View Only Oct 18, 2016
@sockeqwe
Copy link
Owner

sockeqwe commented Oct 19, 2016

Hi,
I'm glad you want to contribute! Well, I also caught myself implementing this multiple times, but I would like to avoid putting this into this library, for various reasons:

  1. maintaining & testing
  2. you never catch all use cases and users will start to complain about it. See Error View as a View in MvpLce implementations? #132 Allow non textview errorview in LceAnimator.showErrorView #107 some problem about error view in MvpActivity #42 about LCE components
  3. I would prefer to keep this library small and concise. See Provide MvpSupportMapFragment #143 about adding support for Map Fragments. The problem is if we add SE, then next month we may have to add another XY component too and so on.

I'm open to discuss that, but I think it would be a better approach to write and publish a standalone SE library on top of Mosby. I would be more than happy to share the link to such a extension module library in the README. See https://github.com/Bodo1981/appkit

What do you think?

By the way: In my experience you need 3 states i.e. for login: 1. Show login input form. 2. Show loading (Sending) 3. Show Error ( and optionally 4. Show successful)

@leonardo2204
Copy link
Contributor Author

Sure, no problem.
I see your point and I agree that people may complain about some tweaks it should have or use cases it wouldn't cover. Nevertheless I think it's a very common use case, and I also get myself implementing this over and over. I'm not sure if it's a good approach to create a lib on top of that just for this feature. If so, LCE should also be a part lib, right ?

About the states, I agree 100%, that' how I do on my side. And I'd give a +1 for the step 4, show successful callback.

Thanks for taking your time and I really appreciate all the work and effort you put on your project dude !

@leonardo2204
Copy link
Contributor Author

leonardo2204 commented Oct 20, 2016

Hey @sockeqwe, how is it going ?
I came up with something like that https://github.com/leonardo2204/mosby.git

See what your toughts are on that !

@sockeqwe
Copy link
Owner

Sorry, I am a little bit busy right now, I will take a detailed look during the weekend...

Indeed, there are discussions going on to remove LCE from this project (or at least package it into is own artifacts) #148 and I think if I would rewrite this project from scratch I wouldn't add LCE into this project.

Usually, some other Mosby users give their feedback here on github on some project related decisions. I think we should wait some more days, maybe we get some more opinions about that, and then we make a final decision. I'm open to discuss that with the community and you are part of the community!

I'm not sure if it's a good approach to create a lib on top of Mosby

I'm not really talking about writing a whole big library like appkit, I'm just saying that instead of putting all you code into Mosby directly, we could simple publish those classes as its own artifact (with having a dependency to Mosby).

Does that all makes sense for you?

@leonardo2204
Copy link
Contributor Author

Sure, no problem !
Ohh I see it, so that would be something like the viewstate separate lib, right ? I misunderstood haha. In that case I think that would be just fine, also we could even extract the LCE part into the same package.. What do you think ?
We'd have something like a MosbyForm, something like that (I'm not good at naming things).

Thanks for your time, I'll wait for some other feedback ;)

@dimsuz
Copy link

dimsuz commented Oct 20, 2016

We are using mosby in several projects and in each of them we have different requirements for LCE views which happen to differ a bit from each other. And also from Mosby's default implementation of LCE. The concept is there, but I discovered that it is sometimes easier to create my own implementation of LCE stuff than to try to customize mosby's and to apply workarounds for functionality it does a little bit differently. For example mosby has the concept of pullToRefresh (last time I looked) while I might not need it, etc etc.

So I feel that it would be a correct move - to separate this into a different artifact which would provide this functionality for the projects that can use it right away. And other implementations can use that for a reference and inspiration also :)

@DrewCarlson
Copy link

I also use multiple variations of LCE for different cases and some are the Mosby provided implementation. To the same end I have two implementations around forms as well.

I'm onboard with moving LCE and adding other things like SE to a separate module, that makes the most sense to me.

@leonardo2204
Copy link
Contributor Author

leonardo2204 commented Nov 11, 2016

I kinda stopped developing this features waiting for a feedback from you guys...
More thoughts on how to design this new features ?

@sockeqwe
Copy link
Owner

I think we can add this but in a separate module in Mosby 3.0 . Still not sure about the naming...

@leonardo2204
Copy link
Contributor Author

@sockeqwe Thanks for the feedback !
Sure, do you have any naming sugestion ?
As soon as I have some more time, I'll keep implementing !

@sockeqwe
Copy link
Owner

Regarding naming: Content Loading Error Success (CLES) ? Doesn't sound that nice in my ears but nothing better came into my mind until now ... not sure ...

@leonardo2204
Copy link
Contributor Author

leonardo2204 commented Nov 15, 2016

@sockeqwe Hmm, I'm not sure about that, IMO, it doesn't seem like a "natural" name to refer to Form views. Maybe using send or something else to make it clearer ? Something like Send Loading Error Success (SLER) or Form Loading Error Success (FLES) ??
And also, as it would serve as a Form View, does it make sense to have a Content in this case ? I mean, the content is the Form, so I 'm not sure if we would ever need to hide/show its content.
Thanks again for your reply !

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

No branches or pull requests

4 participants