-
Notifications
You must be signed in to change notification settings - Fork 49
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 #758
Conversation
Signed-off-by: Qin <[email protected]>
Signed-off-by: Qin <[email protected]>
Signed-off-by: Qin <[email protected]>
Signed-off-by: Qin <[email protected]>
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 think that maybe, here, you could make entityDisplay
optional and display the provided string (for CP, SP and GP) else fall back to "business". What do you think?
Signed-off-by: Qin <[email protected]>
Agree, thanks! I updated the codes |
/gcbrun |
Temporary Url for review: https://business-create-dev--pr-758-as4ni3qx.web.app |
@@ -22,7 +22,7 @@ | |||
"@bcrs-shared-components/base-address": "2.0.3", | |||
"@bcrs-shared-components/breadcrumb": "2.1.15", | |||
"@bcrs-shared-components/business-lookup": "1.3.4", | |||
"@bcrs-shared-components/certify": "2.1.51", | |||
"@bcrs-shared-components/certify": "2.1.55", | |||
"@bcrs-shared-components/completing-party": "2.1.30", |
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.
The current dev correctly display the required wording ("are" authorized). Maybe we don't need to update it to 2.1.55.
But when I run locally, it displays "is" instead of "are".
Could you please suggest, 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.
Check your node_modules folder. You might have an older version than 51.
As for whether to upgrade from 51 to 55...
- on the one hand, it's good to keep these imports updated
- on the other hand, did very much change from 51 to 55? if so then extra testing is needed
If there are no significant changes from 51 to 55 then I leave it up to you whether to upgrade 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.
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.
That likely means some dependency was updated (eg, enums or interfaces).
Up to you whether to update to 55 or not.
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.
So I leave it upgraded
Well, darn! That's even more changed files. But I prefer this as it's DRYer and cleaner. Thanks for the update! |
Signed-off-by: Qin <[email protected]>
/gcbrun |
Temporary Url for review: https://business-create-dev--pr-758-as4ni3qx.web.app |
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).