-
Notifications
You must be signed in to change notification settings - Fork 45
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
16995 - Change Store Actions Types #519
Conversation
@@ -34,7 +34,7 @@ export interface NameRequestIF { | |||
details?: NameRequestDetailsIF | |||
expiry?: string | |||
filingId?: number | |||
legalName: string | |||
legalName?: string |
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 is needed to prevent a type error in filing-template-mixins.ts
at FilingTemplateMixin::parseCorrectionFiling
. When it tries to store the name request with this.setNameRequest
, the filing.correction.nameRequest
has legalName
as an optional value.
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.
OK
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.
Leo, you can resolve this conversation (unless you want it visible still).
this.stateModel.tombstone.userInfo = userInfo | ||
}, | ||
setOrgInfo (orgInfo) { | ||
setOrgInfo (orgInfo: any) { |
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.
Should I create a new interface for orgInfo (and userInfo) to avoid using any
? The corresponding get call also uses any
.
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.
Unless other reviewers disagree, I believe any
here should be OK.
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 and userInfo above are declared as any
in the TombStoneIF, so this is OK with me.
Ideally, we'd have an interface for both of these, but that's outside the scope of this ticket so don't worry about it.
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.
Leo, you can resolve this conversation (unless you want it visible still).
Codecov Report
@@ Coverage Diff @@
## main #519 +/- ##
==========================================
+ Coverage 87.11% 87.20% +0.08%
==========================================
Files 207 208 +1
Lines 3919 3938 +19
Branches 510 511 +1
==========================================
+ Hits 3414 3434 +20
+ Misses 491 490 -1
Partials 14 14
|
@severinbeauvais Can you add some reviewers to this PR? Thanks |
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.
LGTM Leo (after fixing comments and rebasing)! 👍 Awesome stuff.
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 looks amazing! (I love variable/function typing.) Good job. Just a couple small comments.
Issue #: /bcgov/entity#16995
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the bcrs-entities-create-ui license (Apache 2.0).