-
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
22639 - Update Certify Section Wording #598
Conversation
Signed-off-by: Qin <Arwen.Qin@gov.bc.ca>
Signed-off-by: Qin <Arwen.Qin@gov.bc.ca>
Signed-off-by: Qin <Arwen.Qin@gov.bc.ca>
Signed-off-by: Qin <Arwen.Qin@gov.bc.ca>
"resolved": "https://registry.npmjs.org/@bcrs-shared-components/certify/-/certify-2.1.5.tgz", | ||
"integrity": "sha512-+/6MRAHt1xRTcIauIg6lWDII9sLovl0rr2zAsV3+8zvm9+MVujNqC7mlG0CHI2Czl/tqT+CxpAhm/7m6pOk0+g==", | ||
"version": "2.1.55", | ||
"resolved": "https://registry.npmjs.org/@bcrs-shared-components/certify/-/certify-2.1.55.tgz", |
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.
There have been a lot of changes since this shared component was first imported! Please test Edit UI well in case there were some incompatible changes.
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.
Just tested it, looks good!
"dependencies": { | ||
"@bcrs-shared-components/corp-type-module": "^1.0.16", | ||
"@bcrs-shared-components/enums": "^1.1.15", | ||
"vue": "^2.7.14" | ||
} |
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 ran npm install @bcrs-shared-components/certify, it added those 2 dependencies, is it correct?
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.
Yes!
This is because the Certify component depends on Vue and Interfaces:
https://github.com/bcgov/bcrs-shared-components/blob/main/src/components/certify/package.json
And Interfaces depends on Corp Type Module and Enums:
https://github.com/bcgov/bcrs-shared-components/blob/main/src/interfaces/package.json
It's listing Vue as "2.7.14 or greater". Hopefully that resolves to 2.7.16 (same as Edit UI) instead of importing 2 separate versions of Vue.
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.
Anyway, I think this is OK.
Signed-off-by: Qin <Arwen.Qin@gov.bc.ca>
/gcbrun |
Temporary Url for review: https://business-edit-dev--pr-598-f03y4ktw.web.app |
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.
Looks good.
I looked for a better way to do this (instead of updating 32 resource files), such as displaying "business" by default, but that would break CertifySection.vue::readableEntityType(), which needs to handle its current fallback for SPs and GPs. So, let's leave it as-is.
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 tested a few filings but not all. Looks good in my tests
Issue #: /bcgov/entity#22639
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).