-
Notifications
You must be signed in to change notification settings - Fork 31
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
Residential Exemptions in MHR table #1584
Conversation
/gcbrun |
Temporary Url for review: https://bcregistry-assets-dev--pr-1584-c0u4dv1t.web.app |
Codecov Report
@@ Coverage Diff @@
## main #1584 +/- ##
==========================================
+ Coverage 72.35% 77.34% +4.99%
==========================================
Files 307 379 +72
Lines 12767 6995 -5772
Branches 2630 1115 -1515
==========================================
- Hits 9237 5410 -3827
+ Misses 3518 1553 -1965
- Partials 12 32 +20
Flags with carried forward coverage won't be shown. Click here to find out 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.
LGTM.
<v-list-item | ||
@click="handleAction(item, TableActions.REMOVE)" | ||
data-test-id="remove-mhr-row-btn" | ||
> |
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 will have some conflicts here, looks like several changes in this area overlap.
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 can resolve, as your PR is bigger and should go first.
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 to me!
That being said, whomever goes in 2nd will have some conflicts to resolve in the TableRow.
Have a look at my TableRow changes and let me know what you think, i'm good with either or 👍
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 great
5a74ea1
to
197b40c
Compare
/gcbrun |
@cameron-eyds sync'ed with main, resolved conflicts... if you want to take another quick look. |
Temporary Url for review: https://bcregistry-assets-dev--pr-1584-c0u4dv1t.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.
Still looks good :)
197b40c
to
4f12c07
Compare
/gcbrun |
Temporary Url for review: https://bcregistry-assets-dev--pr-1584-c0u4dv1t.web.app |
4f12c07
to
94db353
Compare
/gcbrun |
Temporary Url for review: https://bcregistry-assets-dev--pr-1584-c0u4dv1t.web.app |
@@ -820,14 +820,14 @@ export default defineComponent({ | |||
const today = new Date() | |||
const expireDate = new Date() | |||
// expireDate.setDate(expireDate.getDate() + days) | |||
var dateExpiry = moment(new Date( | |||
var dateExpiry = moment.utc(new Date( |
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 the change I made (+ unit test update) for fix the dates issue. @cameron-eyds
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'm not fully familiar with moment or the implications but if you are confident than so am i :)
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 verified that before and after this change, the number of days to expire was the same. Also, we are using UTC format in the Date()
inside the moment function.
028487b
to
eb5e7f6
Compare
Issue #:
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 PPR license (Apache 2.0).