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

Worked on issue #1215 #1456

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

sachdevlaksh
Copy link

What kind of change does this PR introduce? (delete not applicable)

  • Feature

Fixes #1215

@thegrims
Copy link
Collaborator

Hi @sachdevlaksh thanks for your contribution, it looks like the initial needs to be added to the SpouseAndDependent return value

11:18:59 AM: Failed to compile.
11:18:59 AM: 
11:18:59 AM: /opt/build/repo/src/components/TaxPayer/SpouseAndDependent.tsx
11:18:59 AM: TypeScript error in /opt/build/repo/src/components/TaxPayer/SpouseAndDependent.tsx(72,3):
11:18:59 AM: Property 'initial' is missing in type '{ role: PersonRole.DEPENDENT; qualifyingInfo: { numberOfMonths: number; isStudent: boolean; }; dateOfBirth: string; relationship: string; firstName: string; lastName: string; ssid: string; isBlind: boolean; }' but required in type 'Dependent<string>'.  TS2741
11:18:59 AM:     70 |   }
11:18:59 AM:     71 |
11:18:59 AM:   > 72 |   return {
11:18:59 AM:        |   ^
11:18:59 AM:     73 |     ...rest,
11:18:59 AM:     74 |     role: PersonRole.DEPENDENT,
11:18:59 AM:     75 |     qualifyingInfo: {
11:18:59 AM: 

@sachdevlaksh
Copy link
Author

Hi @sachdevlaksh thanks for your contribution, it looks like the initial needs to be added to the SpouseAndDependent return value

11:18:59 AM: Failed to compile.
11:18:59 AM: 
11:18:59 AM: /opt/build/repo/src/components/TaxPayer/SpouseAndDependent.tsx
11:18:59 AM: TypeScript error in /opt/build/repo/src/components/TaxPayer/SpouseAndDependent.tsx(72,3):
11:18:59 AM: Property 'initial' is missing in type '{ role: PersonRole.DEPENDENT; qualifyingInfo: { numberOfMonths: number; isStudent: boolean; }; dateOfBirth: string; relationship: string; firstName: string; lastName: string; ssid: string; isBlind: boolean; }' but required in type 'Dependent<string>'.  TS2741
11:18:59 AM:     70 |   }
11:18:59 AM:     71 |
11:18:59 AM:   > 72 |   return {
11:18:59 AM:        |   ^
11:18:59 AM:     73 |     ...rest,
11:18:59 AM:     74 |     role: PersonRole.DEPENDENT,
11:18:59 AM:     75 |     qualifyingInfo: {
11:18:59 AM: 

I see. Thanks for the update. Right now my local UsTaxes setup is not able to detect CommonTests.ts as test class, so not able to find root cause in test cases failure.

@sachdevlaksh
Copy link
Author

@thegrims , Did you get chance to look into PR.

@thegrims
Copy link
Collaborator

@sachdevlaksh the UI looks good, I ran the unit tests, and it looks like the first name check needs to be updated, and some other errors need to be fixed. You can validate locally by running npm test

Copy link
Collaborator

@thegrims thegrims left a comment

Choose a reason for hiding this comment

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

failing unit tests

@sachdevlaksh
Copy link
Author

@thegrims I fixed two test error, which i could easily find it was because of my new changes, for the rest two I need your help, want to know what is these all about.
Screenshot 2022-10-24 at 7 58 53 PM

@@ -22,7 +22,8 @@ import ListItemText from '@material-ui/core/ListItemText'
import PersonIcon from '@material-ui/icons/Person'

export const labels = {
fname: 'First Name and Initial',
fname: 'First Name',
initial: 'Initial',

Choose a reason for hiding this comment

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

Would this be more clear if it was called 'Middle Initial'?

Copy link

Choose a reason for hiding this comment

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

Agreed, midInitial: 'Middle Initial' would likely be better naming for the key and the label.

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.

Create separate field for initial
4 participants