-
Notifications
You must be signed in to change notification settings - Fork 1
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
Ipc 160 customize review screen #387
Ipc 160 customize review screen #387
Conversation
… to match design
- cleanup `styles` file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Some minor changes are needed.
Please also merge ipc-health-sdk-update
into this branch. It will fix the tests.
I also left some observations that could be taken care of in a comment in the ticket. We can talk about them tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alpar has mentioned everything. just one thing I can add. We are supporting dark mode with the help of colors. In Health sdk, we are using the Theme to do so! I would go with the colors, otherwise, we cannot make sure the right color is used everywhere!
- Removed layout related properties from `style` and moved them into the layout itself - Removed unused styles - Removed drag handle from `ghs_fragment_review` IPC-160
… visible - Remove focus from any input field which might be in focus when tapping `Payment` button - Fix new iban not being validated on change IPC-160
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for all the changes! 👏 I left a comment to make sure that the IBAN validation behaviour change is really required.
if (prevPaymentDetails != null && prevPaymentDetails.iban != paymentDetails.iban) { | ||
nonEmptyValidationMessages.remove(ValidationMessage.InvalidIban) | ||
nonEmptyValidationMessages.addAll(validateIban(paymentDetails.iban).filterIsInstance<ValidationMessage.InvalidIban>()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Clarification] I am interested in why you added the IBAN revalidation here. I saw it caused two tests to fail so we intentionally did not revalidate the IBAN here:
I checked iOS and there validation is also repeated only on pay button press. So we need to have a good reason to change this behaviour on Android.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I saw in this comment https://ginis.atlassian.net/browse/IPC-160?focusedCommentId=23915 that the IBAN validation error appears when the field is no longer in focus - this was not happening when I tested, this being the reason I added the revalidation.
I can take it out and leave it as is so we match iOS implementation and keep the tests from failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thank you!
ghs_fragment_review
screen to match Figma design.colors.xml
,dimens.xml
andstyles.xml
a bit.