Error back #44
Error back #44
Conversation
|
Hi @leonardo2204,
|
| app:srcCompat="@drawable/ic_check" | ||
| android:visibility="gone" /> | ||
|
|
||
| <android.support.v7.widget.AppCompatImageView |
There was a problem hiding this comment.
Is a separate ImageView needed here? Maybe we could use ms_stepDoneIndicator and change the drawable + tint? What do you think?
There was a problem hiding this comment.
I was thinking about that when developing, but I kinda came to realize that even tough it's an indicator and they will never show at the same time, it just seems better to have two separate views, to apply styling, show and hide.
But it's easily changeable, if you prefer, I'd can do it :)
There was a problem hiding this comment.
Hmmm you're about the styling. I guess it would be easier to have two separate views for that :)
| <!-- Flag indicating if the Back (Previous step) button should be shown on the first step. False by default. --> | ||
| <attr name="ms_showBackButtonOnFirstStep" format="boolean" /> | ||
|
|
||
| <!-- Flag indicating wheter to keep showing the error state when user moves back. Only available with 'tabs' type. False by default. --> |
There was a problem hiding this comment.
Nice catch haha, thanks !
| } | ||
|
|
||
| //if moving forward and got no errors, set hasError to false, so we can have the tab with the check mark. | ||
| if(mStepperType instanceof TabsStepperType) |
There was a problem hiding this comment.
We could replace the if statements with polymorphism i.e. this could be changed to
mStepperType.setErrorStep(mCurrentStepPosition, flag)
by default this would do nothing, but for TabsStepperType this would call:
mTabsContainer.setErrorStep(stepPosition, false);
There was a problem hiding this comment.
I've given this a lot of thought also, my solution really is not the best, but I was thinking if adding a flag just to use in one place a good approach here...
Also, it didn't seem to me that in the near future a new kind of tab or something like it would be needed, making a lot of spaghetti 'if's'.
Also it's a no problem to change it !
There was a problem hiding this comment.
Hi,
I'm considering adding the alternative tabs look so it might be a pain in the a*s to update all those ifs :/
Could you replaced them with the proposed solution so that it's easier to maintain it later?
|
About your points:
|
|
Think I got it all covered on my last commit ! |
|
Hi @leonardo2204, I've also noticed that point 2. from my previous comment is still not covered? |
|
Hi @leonardo2204, |
|
Hey @zawadz88 Cool, thank you ! |
I created this PR to address #32 enhancement.
My previous code from the #37 is also here, so, sorry for the long commit.
I'd suggest taking a look at 2c33eaf for code only of the #32 issue.
I managed the issue addressed by @zawadz88 :
Adding a
showErrorStateOnBackflag, so the user can choose whether or not to clean the error from the current step when moving back.The main point I'd like to discuss is when we get an error on "complete" state, do all the necessary steps to get it right and press "complete" again. Currently the callback is correctly fired, but the "error" state is never cleared. Not sure if this should be treated, as most of the cases the user will move forward to another screen.
What are your thoughts on that ?
Thanks!