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

Feat: #896 Add error message for all fields in RegistrationFragment #907

Closed
wants to merge 1 commit into from

Conversation

letelete
Copy link
Contributor

@letelete letelete commented Oct 24, 2018

Add error message for all fields.

Input error preview

input_error_presentation

build.gradle Outdated
@@ -19,8 +19,8 @@ buildscript {

allprojects {
repositories {
jcenter()
google()
Copy link
Collaborator

Choose a reason for hiding this comment

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

@letelete Undo this change

Copy link
Contributor

Choose a reason for hiding this comment

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

Pull request #909 covers this change, and a fix for multidex

@@ -1,5 +1,7 @@
package org.mifos.mobilebanking.ui.views;

import android.widget.EditText;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@letelete Undo this change

@miPlodder
Copy link
Collaborator

miPlodder commented Oct 24, 2018

@letelete Fix the Travis Build failed and add proper GIF showing UI changes.
Moreover, instead of creating multiple commits for small functionality change, create a single commit for the issue.

@letelete
Copy link
Contributor Author

Is there any way to edit these, already existing commits and squash them all into one? I'm not sure how can I could've done that. Thought it's being done by a person who's merging the PR.

@letelete
Copy link
Contributor Author

letelete commented Oct 24, 2018

@miPlodder travis failed with Execution failed for task ':app:getDependencies'., which I've struggled with when first time cloning a repo. I fixed it on my machine by moving google() before jcenter() in build.gradle

@letelete letelete mentioned this pull request Oct 24, 2018
3 tasks
@miPlodder
Copy link
Collaborator

@letelete You can use this link to squash the commit to one. Its job of the developer to maintain a clean Commit history.

@miPlodder Travis failed with Execution failed for task ':app:getDependencies'., which I've struggled with when first time cloning a repo. I fixed it on my machine by moving google() before jcenter() in build.gradle

Create an issue for Travis Build failed to explain what's happening. I will look into it.

@@ -115,61 +151,153 @@ public void registerClicked() {
}

private boolean areFieldsValidated() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@letelete What you are doing is correct. But instead of creating methods for every if, write them in a single method like it was written before.

Copy link
Contributor Author

@letelete letelete Oct 25, 2018

Choose a reason for hiding this comment

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

@miPlodder Isn't it a bad practice to create long methods?

It's so unreadable and hard to maintain if the single method contains all of the validation logic. It's also easy to repeat yourself here, and that's what I've seen in your code before my fix. There were 2 validations for some of the input (I guess it was a password) which was checking if password length is greater than a) 5, b) 6 which is complete nonsense and it 100% comes out of hard-to-read method with tons of validations.

I've also seen validations for some specific inputs which were not grouped-up so at the beginning I could spot X validation and at the end of the method, I could also spot validation for the same X input.

My solution ensures clean-code standards. Methods are easy to maintain now and if we want to change some of the validation we just need to search for a specific method instead of analyzing one, enormous method.

Splitting validations into method has also another positive aspect, which is a fact, that with the live-update feature, we don't need to check every single validation and just the one we are interested in(validation for currently edited input).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@letelete I know that your code is cleaner. But I have a better alternative which I will add to the GCI tasks in future.
We can create a Utils Class where we can add all the validations methods (like you added here). By that way, we can call those methods, for any of the EditText (like Sign up, Login) where that validation is required. This will reduce redundant code for validation.

Copy link
Contributor Author

@letelete letelete Oct 25, 2018

Choose a reason for hiding this comment

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

Yeah, that's actually a good idea which I was thinking about when faced with writing the same code for login_activity validation(#911) 😆 I will get all of these and split them into one as it was before, then.

@letelete
Copy link
Contributor Author

letelete commented Oct 25, 2018

I've added an issue describing the build problem #910

@letelete
Copy link
Contributor Author

letelete commented Oct 25, 2018

Fixing the bug described in #910 makes build successful. I thought you would like me to remove gradle.build change since It's not the PR for that.

@letelete letelete force-pushed the issue#869 branch 2 times, most recently from b6ecc5a to 09e0722 Compare October 25, 2018 14:35
@letelete
Copy link
Contributor Author

@miPlodder Done. Btw. I've removed live update feature for now and will add it again when Utill class is ready.

@letelete letelete changed the title Feat: #896 Add error message for all fields, add live update validation Feat: #896 Add error message for all fields Oct 25, 2018
@letelete letelete changed the title Feat: #896 Add error message for all fields Feat: #896 Add error message for all fields in RegistrationFragment Oct 25, 2018
@@ -96,12 +99,6 @@ public void registerClicked() {
payload.setFirstName(etFirstName.getText().toString());
payload.setLastName(etLastName.getText().toString());
payload.setMobileNumber(etPhoneNumber.getText().toString());
if (!etPassword.getText().toString().equals(etConfirmPassword.getText().toString())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@letelete Good fix here. 👍

@letelete letelete force-pushed the issue#869 branch 2 times, most recently from f188144 to a75fd66 Compare October 27, 2018 22:10
@letelete
Copy link
Contributor Author

@miPlodder Add request view feature which I told you about earlier. Speaking about live update validation. I would like to add it later when we will create separate validation class which you were talking about.

Preview

preview

…atures

Add error display for every input field. All of them can appear at once now.
Add feature which find the first invalid input and force request on it with automatically appearing keyboard to help user correct it instantly.
Fix validation logic.
Add live update feature
@letelete
Copy link
Contributor Author

letelete commented Oct 31, 2018

Add new validation logic

  • If the input has focus for the first time:
    • don't use live-update validation
  • If user left inputX (and move to inputY):
    • check correctness of inputX
      • if invalid - show error
  • If user back to the previous input (in this case: from inputY to inputX) AND input is invalid:
    • turn on live-update
  • if user back to the previous VALID input
    • don't use live-update validation
  • On register button click:
    • show error for every invalid input
    • turn on live-update validation for every input
    • move to the first invalid input and request focus on it
    • force keyboard

11

22

33

@miPlodder
Copy link
Collaborator

@letelete You logic is inconsistent. Try different combination on the device. You can work on this in your spare time. You can focus on newer tasks.

@miPlodder
Copy link
Collaborator

@letelete Good job, but we are coming up with new design for Sign-Up in which we are splitting the fields in different fragments which makes this PR obsolete. Anyways good job here.

@miPlodder miPlodder closed this Jan 1, 2019
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.

3 participants