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

Tbolt/3498 Other state expenses validation #3957

Merged
merged 227 commits into from
May 18, 2022

Conversation

tbolt
Copy link
Contributor

@tbolt tbolt commented Apr 8, 2022

Resolves #3498

Description-

  • Updates other state expenses section to use new form validation tools

This pull request is ready to review when...

  • Automated tests are updated (and all tests are passing)
  • The change has been documented
  • Associated OpenAPI documentation has been updated
  • Changelog is updated as appropriate
  • The experience passes a basic manual accessibility audit (keyboard nav, screenreader, text scaling) OR an exemption is documented

This pull request can be merged when…

  • Code has been reviewed by someone other than the original author
  • QA has verified the accessibility and functionality related to the change
  • Design has approved the experience
  • Product has approved the experience

Sorry, something went wrong.

@mirano-darren
Copy link
Contributor

Well done!

Given When Then Result
other state expenses subform form is filled out with valid fields save button is clickable and saves PASS ✅
other state expenses subform right after pressing "Add State Expense" and no fields are touched save button is disabled PASS ✅
other state expenses subform tab navigating through the form, not filling anything out validation error appears on all fields PASS ✅
an existing other state expenses subform form is edited, click cancel changes are not saved PASS ✅
an existing other state expenses subform form is edited, click save changes are saved PASS ✅
an existing other state expenses subform a field is edited to be invalid inline validation triggers and save button is disabled PASS ✅

Base automatically changed from tbolt/3866-joi-state-staff to main May 9, 2022 14:15
@jeromeleecms jeromeleecms added the feature PR label for release log label May 9, 2022
@thetif thetif requested review from beparticular and jeromeleecms and removed request for Sun-Mountain and itsonlydio May 12, 2022 15:25
Copy link
Contributor

@jeromeleecms jeromeleecms left a comment

Choose a reason for hiding this comment

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

Besides the system wide bug on subforms filed for #4047, this works as advertised. Gtg from my end.

Copy link

@beparticular beparticular left a comment

Choose a reason for hiding this comment

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

Steps: Expected:
Don't select an expense category
Fill in all the other fields, can't save ofc
Go back to the expense category and select an option
Unexpected: the validation error is still there after I've chosen an option.

If I tab or click out of the field it updates and there is no longer an error, but the expected behavior would be that selecting an option would fix the problem and the error, rather than selecting an option and needing to navigate to another field.

@tbolt
Copy link
Contributor Author

tbolt commented May 17, 2022

@beparticular My understanding was that the team decided to only trigger validation "onBlur." Which means that even though you select an item from the dropdown you still have the field "focused" and only when you exit (tab or mouse click away from it) that would trigger the validation. We can revisit and make the call to have some exceptions because I agree the current behavior doesn't feel expected. We could switch to doing validation anytime a change is made for all inputs/fields. This would fix the issue you pointed out but also impact all other inputs. So for text inputs, as soon as you typed one character we would validate.

@tbolt tbolt requested a review from itsonlydio May 17, 2022 20:00
Copy link
Contributor

@itsonlydio itsonlydio left a comment

Choose a reason for hiding this comment

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

The Other state expenses field validations works!

@tbolt tbolt merged commit 7d10733 into main May 18, 2022
@tbolt tbolt deleted the tbolt/3498-joi-other-state-expenses branch May 18, 2022 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature PR label for release log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Update Other State Expenses
8 participants