-
Notifications
You must be signed in to change notification settings - Fork 27
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
Issue 442/basic information form #533
Conversation
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.
This all looks like it's on the right track.
- For the placeholder text, I'd say legal first name and legal last name. We want to emphasize that the data put here should be usable in a legal context.
- I think there's a typo in the test that's causing it to fail. You're trying to select a textbox role, but the first name is actually an input role.
I'm not really sure why but two things:
|
Co-authored-by: Tim Standen <37914436+timbot1789@users.noreply.github.com>
…/codeforpdx/PASS into issue-442/basic-information-form
…/codeforpdx/PASS into issue-442/basic-information-form
Opening up this PR now. That final test still isn't passing, so my hope right now is to comment that one out and create a separate issue to address it. Also please ignore my above mis-click. |
Think we could make an exception for this one. We could comment on the unit test for the basic information form for now, but like what you've mentioned we'll have to open an issue/bug report to fix it. Can approve this after the merge conflict resolution. |
…/codeforpdx/PASS into issue-442/basic-information-form
|
||
expect(legalFirstName.value).toBe(''); | ||
expect(legalLastName.value).toBe(''); | ||
expect(legalDOB.value).toBe(null); |
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.
I believe this test is failing because the input field for the date picker is not your typical text field. You can't use the same approach to test the data picker. The same would be said for legalGender
which is a select field.
If you'd like, we can open up another issue to write unit tests for these inputs.
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.
Hey Andy, I've finished reviewing the PR. Just one minor feedback on potential changes (see feedback), but it seems to be working overall, so I'm approving.
firstName: '', | ||
lastName: '', | ||
dateOfBirth: null, | ||
gender: '' |
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.
Not too big of an issue since the feature appears to be working as intended, however, MUI is complaining about the input fields if it's not using one of the values from the option elements.
Think we could simply set the defaultValue, state, and clear value for gender as the number 9
to work around the problem. So Line 36, Line 75, and Line 133.
This PR:
Resolves #442
Just a draft for now to make sure I'm on the right track.
1. Adds (basic) styling to
BasicInfo.jsx
2. Adds logic from #515
Screenshots (if applicable):
Future Steps/PRs Needed to Finish This Work (optional):
1. Ensure this component is properly implementing the
useCivicProfile
hook2. Ensure the labeling (such as all the
id
s andname
s) is consistent and adheres to any standards (if necessary)Issues needing discussion/feedback (optional):
1. What styling is needed for each form as well as overall Civic Profile section