-
Notifications
You must be signed in to change notification settings - Fork 120
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
Disabling the "Other" Option Now Disables the Other Text and Hides the Keyboard #2528
Conversation
* Initial Commit * Working - 1 * Small change * Updated Code based on the requirements * Removed Signout from NavDrawer * Modified code to retrieve user details using authentication manager
Fixed this issue: Disabling "other" option doesn't hide/disable "Other" text
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.
Can we have a unit test to test this scenario?
@NudurupatiSurya I believe to test this scenario you would want to add a UI test to |
Yes, |
Hi @NudurupatiSurya |
Gentle Reminder @NudurupatiSurya |
|
Hi, I wanted to give you a quick update. Before adding the new tests, I ran the existing ones in
I am currently working on resolving this and adding the new tests as suggested. I'll reach out if I get stuck anywhere. I should have an update for you later this week. Thanks! |
All the tests are passing for me. Could you please tell me how you are running the test cases? |
Hi @NudurupatiSurya |
@NudurupatiSurya Updated branch here and rerunning test on CI to keep things moving foward. /gcbrun |
@anandwana001 Confirmed tests are failing with the following when merged with HEAD:
@NudurupatiSurya Any updates? |
ok, I have the latest update, and tests are passing for me. |
Hi @gino-m & @anandwana001, I am currently in the process of some job interviews, so I couldn't reply back promptly. @anandwana001, even after updating my current branch, these tests are failing I think the tests are failing because the condition I added is overriding some of the otherText's previous behavior of selecting the other radio button when the user selects All the failed tests are trying to write something in I will update my logic so that these tests will not fail and Thank you for your understanding. |
Hi @NudurupatiSurya any updates? |
@NudurupatiSurya Are you able to finalize this PR within the next week or so? The following tests are failing:
|
I ran locally and verified that tests are still in fact failing, and that the behavior of this PR doesn't match what's being asked in #2399. I'll revise that ticket to make what's required more clear. |
Fixes #2399
Demo:
Screen_recording_20240628_123808.mp4
@... PTAL?