-
Notifications
You must be signed in to change notification settings - Fork 69
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
21813 Added Continuation Authorization Review page, route, etc #2894
Conversation
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.
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 copied right from Create UI.
@@ -0,0 +1,126 @@ | |||
<template> | |||
<div | |||
v-if="!!continuationIn" |
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.
Don't render this component until/unless we have data.
I should be able to remove some of the conditional chaining (a?.b
) with this.
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've decided to keep the conditional chaining in place, so that the computeds don't depend on the template. (Ie, the computeds are only evaluated when referenced in the template, and the template won't render without the main object.)
@@ -0,0 +1,387 @@ | |||
<template> | |||
<div | |||
v-if="!!continuationIn" |
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.
Don't render this component until/unless we have data.
return ( | ||
entityType === CorpTypes.ULC_CONTINUE_IN && | ||
country === 'CA' && | ||
(region === 'AB' || region === 'NS') |
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.
We don't have constants for countries or regions and I think that's overkill for here.
path: '/staff/continuation-review/:reviewId', | ||
name: 'ContinuationReview', | ||
component: ContinuationAuthorizationReview, | ||
props: true, |
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 passes the variable in the path (reviewId) into the component as a prop.
- imported shared jurisdictions - imported lodash isDate + type - added CSS variable $px-15 - added CardHeader.vue - added ExtraprovincialRegistrationBc.vue - added HomeJurisdictionInformation.vue - added continuation review interfaces - added route - added fetchContinuationReview() (with mocked data) - added downloadDocument() - added DateUtils class - added ContinuationAuthorizationReview.vue - updated browserslist - added unit tests
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.
✓ tests/unit/components/ContinuationAuthorizationReview.spec.ts (8)
✓ ExtraprovincialRegistrationBc component (8)
✓ got the prop
✓ fetched the continuation review object
✓ computed "isExpro"
✓ rendered the component
✓ rendered the error dialog
✓ rendered the container header
✓ rendered the first v-card
✓ rendered the second v-card
Test Files 1 passed (1)
Tests 8 passed (8)
Start at 10:26:25
Duration 2.32s (transform 1.15s, setup 824ms, collect 665ms, tests 110ms, environment 236ms, prepare 144ms)
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.
✓ tests/unit/components/ExtraprovincialRegistrationBc.spec.ts (6)
✓ ExtraprovincialRegistrationBc component (6)
✓ got the prop
✓ rendered the component
✓ rendered all the articles
✓ rendered the first article
✓ rendered the second article
✓ rendered the third articles
Test Files 1 passed (1)
Tests 6 passed (6)
Start at 10:27:04
Duration 2.01s (transform 985ms, setup 827ms, collect 464ms, tests 63ms, environment 221ms, prepare 156ms)
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.
✓ tests/unit/components/HomeJurisdictionInformation.spec.ts (14)
✓ HomeJurisdictionInformation component (14)
✓ got the prop
✓ computed "isContinuationInAffidavitRequired"
✓ rendered the component
✓ rendered the error dialog
✓ rendered all the articles
✓ rendered the first article
✓ rendered the second article
✓ rendered the third article
✓ rendered the fourth article
✓ rendered the fifth article
✓ rendered the sixth article
✓ rendered the seventh article
✓ rendered a functional affidavit download button
✓ rendered a functional authorization download button
Test Files 1 passed (1)
Tests 14 passed (14)
Start at 10:27:33
Duration 2.26s (transform 1.05s, setup 845ms, collect 568ms, tests 154ms, environment 235ms, prepare 95ms)
/gcbrun |
// 2. compute the offset between UTC and Pacific timezone | ||
// 3. add the offset to convert the date to Pacific timezone | ||
// Ref: https://stackoverflow.com/questions/15141762/ | ||
const date = new Date(Date.UTC(year, month, day, hours, minutes)) |
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.
Use moment/or library, no point in reinventing the wheel
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.
Moment is pretty heavy (though it's already imported in this UI).
This is code from our "legacy" date library. JS is enough. Don't need moment 😛
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.
Don't need your JS library, there are professionals that do this for a living
there are plenty of other ones
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.
|
||
get isExpro (): boolean { | ||
const mode = this.continuationReview?.filing?.continuationIn?.mode | ||
return (mode === 'EXPRO') |
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.
Enum?
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.
// use Navigator.msSaveOrOpenBlob if available (possibly IE) | ||
// warning: this is now deprecated | ||
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/msSaveOrOpenBlob | ||
if (window.navigator && window.navigator['msSaveOrOpenBlob']) { |
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.
We don't support IE 11?
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 copied this from our other UIs.
I see that common-utils.ts has slightly different blob handling. Do you recommend I update the code above?
sbc-auth/auth-web/src/util/common-util.ts
Line 156 in e242563
static fileDownload (data: any, fileName: string, fileType: string = 'text/plain') { |
@@ -0,0 +1,43 @@ | |||
<template> |
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've already added this component with unit tests and everything a while back with unit tests and everything:
https://github.com/bcgov/sbc-auth/blob/main/auth-web/src/components/CardHeader.vue
Keep in mind that my version has extra functionalities/props like a badge.
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. Dunno how I didn't see it.
I'll update this in the follow-on next ticket (bcgov/entity#21815).
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.
No problem! Also, I changed the version here to use composition API to stay consistent in AUTH WEB as per a conversation I had with Travis.
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.
Yup.
I believe we're going to have a chat on Monday about moving some stuff out of Auth Web and into a new UI project. That new UI will probably be Nuxt. When we port code over, it can be converted to Composition API.
Temporary Url for review: https://bcregistry-account-dev--pr-2894-vwhzlljl.web.app SB says, try this: https://bcregistry-account-dev--pr-2894-vwhzlljl.web.app/staff/continuation-review/1234 |
Issue #: bcgov/entity#21813
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 sbc-auth license (Apache 2.0).