-
Notifications
You must be signed in to change notification settings - Fork 42
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
16634 mapped EntityType to CorpTypeCd + cleanup #622
16634 mapped EntityType to CorpTypeCd + cleanup #622
Conversation
- app version = 5.0.3 - added corp-type-module dependency - mapped EntityType to CorpTypeCd
Sorry about the large list of reviewers, but this is a small (but possibly significant) change that I think you should know about, since you are touching, or have recently touched, this repo. Thx. |
@@ -1,3 +1,3 @@ | |||
> 1% | |||
last 2 versions | |||
not ie <= 10 | |||
not dead |
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.
Same as other UI repos.
@@ -1,14 +1,15 @@ | |||
{ | |||
"name": "name-request", | |||
"version": "5.0.2", | |||
"version": "5.0.3", | |||
"lockfileVersion": 2, |
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.
Note that someone, somewhere along the line, did a npm i
while on Node < v16, which changed the lockfile to version 1 in main branch. This is a feature branch and it's OK here. At some point, main branch should be fixed, or maybe this can wait until this feature branch is merged into main in about a month.
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 made these changes to be explicit about the entity type codes used in Namerequest vs. LEAR.
If you look at corp-type-module.ts then you will see that LEAR knows about some NR-only codes, but below you can see there are others. As far as I know, these haven't caused problems between NR and LEAR, but some day they might. And now it's easier to see the difference between the two.
LP = 'LP', | ||
PA = 'PA', | ||
PAR = 'PAR', | ||
BC = CorpTypeCd.BC_COMPANY, |
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 makes things much easier and cleaner for proceeding with the way of navigating work 😁
I think we'll have to change the value of the supported-incorporation-registration-entities
FF we created. Also, I'm going to make the necessary changes in my two tickets to accompany this adjustment after you merge Sev 👍
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. Let me know about that existing FF, and whether we need another one for something different.
I haven't figured out the build issue yet... solved
It seems the build process doesn't want to mix enum-strings with pure-strings, so I did this and it's happy with it:
In other words, the EntityType enum takes values from other enums, instead of a mix of enum/string. |
I have tested this locally and it works fine. |
@@ -87,6 +89,7 @@ export class CommonMixin extends Vue { | |||
isFirm (nr: any): boolean { | |||
return ( | |||
nr?.legalType === EntityType.SP || | |||
nr?.legalType === EntityType.DBA || |
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 sure if this was accidentally missed but it can't hurt to have it. 🤞
- changed some strings to EntityType - worked around EntityType type issue - sorted items in entityTypeCdToText()
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 👍
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 🐱
35a7737
into
bcgov:feature-way-of-navigating
LGTM! |
* - updated browserslist config - app version = 5.0.3 - added corp-type-module dependency - mapped EntityType to CorpTypeCd * - renamed xxx.services -> xxx-services - changed some strings to EntityType - worked around EntityType type issue - sorted items in entityTypeCdToText()
* - updated browserslist config - app version = 5.0.3 - added corp-type-module dependency - mapped EntityType to CorpTypeCd * - renamed xxx.services -> xxx-services - changed some strings to EntityType - worked around EntityType type issue - sorted items in entityTypeCdToText()
* - updated browserslist config - app version = 5.0.3 - added corp-type-module dependency - mapped EntityType to CorpTypeCd * - renamed xxx.services -> xxx-services - changed some strings to EntityType - worked around EntityType type issue - sorted items in entityTypeCdToText()
* - updated browserslist config - app version = 5.0.3 - added corp-type-module dependency - mapped EntityType to CorpTypeCd * - renamed xxx.services -> xxx-services - changed some strings to EntityType - worked around EntityType type issue - sorted items in entityTypeCdToText()
Issue #: bcgov/entity#16634
Description of changes:
Commit 1:
Commit 2:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the namerequest license (Apache 2.0).