-
Notifications
You must be signed in to change notification settings - Fork 47
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
10339 story #5246
10339 story #5246
Conversation
…on review screen;
…ail fields for petitioner and spouse on generated petition; format contact info on generated petition;
@@ -25,7 +25,7 @@ describe('File a petition', () => { | |||
}); | |||
}); | |||
|
|||
it('should display a validaiton error message if user does not select filing type', () => { | |||
it('should display a validation error message if user does not select filing type', () => { |
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.
TY!!
@@ -2395,3 +2395,16 @@ button.change-scanner-button { | |||
.no-wrap-white-space { | |||
white-space: normal; | |||
} | |||
|
|||
.align-items-center { | |||
align-items: center; |
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.
👍👍👍
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.
Nice job Nechama!
filingOptions: { label: string; value: string }[]; | ||
getLetterByIndex: (index: number) => string; | ||
isPetitioner: boolean; | ||
isPractitioner: boolean; |
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.
Is it possible for for both isPetitioner
and isPractitioner
to both be true (or both be false)? If not, would it be better to just have one of these vars?
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.
It’s not possible for both isPetitioner and isPractitioner to be true or both false at the same time, so we could simplify this by having just one property and negating it when necessary. Originally, I used both properties for readability, but I do see that we are negating in some places. Are you okay with this change going in a followup PR along with the cleanup?
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.
Thanks Nechama, yeah I think a followup PR is fine.
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.
Oh and I do like the readability argument, my only concern is us winding in a situation where we wind up in an invalid state. If practical, I think it would be fine to have both props, but have one be a computed prop that is the negation of the other.
}; | ||
}; | ||
|
||
const messages = { | ||
[ROLES.privatePractitioner]: { |
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.
TIL about computed property names. Neat!
No description provided.