Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Home Ownership Component Update - from checkbox to radio group #1622

Merged
merged 15 commits into from
Nov 24, 2023
Merged

Home Ownership Component Update - from checkbox to radio group #1622

merged 15 commits into from
Nov 24, 2023

Conversation

RuoxuanPengBC
Copy link
Contributor

@RuoxuanPengBC RuoxuanPengBC commented Nov 16, 2023 •
edited
Loading

Issue #: /bcgov/entity#18017

Description of changes:

  1. updated home ownership component, from checkbox to radio group
  2. set up validation for the home ownership component
  3. set the default value of ownLand from false to null
  4. updated test suite for the component
  5. minor bug fix for ticket #18249

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).

cameron-eyds
cameron-eyds reviewed Nov 16, 2023
View reviewed changes
ppr-ui/src/components/mhrRegistration/HomeLocation/HomeLandOwnership.vue
height: 60px;
padding: 10px;
margin-right: 0px !important;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be some generic classes to already utilize for these radio button pairs, that match our designs.
If not, no worries.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RuoxuanPengBC

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there are suitable existing radio button class available

cameron-eyds
cameron-eyds approved these changes Nov 16, 2023
View reviewed changes
Copy link
Collaborator

@cameron-eyds cameron-eyds left a 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, just a few comments

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://bcregistry-assets-dev--pr-1622-2u8ir4gq.web.app

@codecov Codecov
Copy link

codecov bot commented Nov 17, 2023 •
edited
Loading

Codecov Report

Merging #1622 (9356d59) into main (9255e77) will increase coverage by 4.86%.
Report is 330 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1622      +/-   ##
==========================================
+ Coverage   72.35%   77.21%   +4.86%     
==========================================
  Files         307      381      +74     
  Lines       12767     7054    -5713     
  Branches     2630     1130    -1500     
==========================================
- Hits         9237     5447    -3790     
+ Misses       3518     1575    -1943     
- Partials       12       32      +20
Flag Coverage Δ
pprui 77.21% <ø> (+4.86%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
ppr-ui/src/App.vue 100.00% <ø> (+56.78%) ⬆️
ppr-ui/src/components/collateral/Collateral.vue 100.00% <ø> (+10.97%) ⬆️
...nents/collateral/generalCollateral/GenColAmend.vue 100.00% <ø> (+18.00%) ⬆️
...onents/collateral/generalCollateral/GenColEdit.vue 100.00% <ø> (+15.15%) ⬆️
...nts/collateral/generalCollateral/GenColSummary.vue 100.00% <ø> (+8.00%) ⬆️
...collateral/generalCollateral/GeneralCollateral.vue 100.00% <ø> (+6.66%) ⬆️
...ts/collateral/vehicleCollateral/EditCollateral.vue 100.00% <ø> (+22.58%) ⬆️
...collateral/vehicleCollateral/VehicleCollateral.vue 100.00% <ø> (+19.41%) ⬆️
...cleCollateral/factories/useCollateralValidation.ts 71.42% <ø> (ø)
...llateral/vehicleCollateral/factories/useVehicle.ts 70.14% <ø> (-0.91%) ⬇️
... and 34 more

... and 283 files with indirect coverage changes

dimak1
dimak1 requested changes Nov 17, 2023
View reviewed changes
Copy link
Collaborator

@dimak1 dimak1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Good job @RuoxuanPengBC. Few comments that require minor changes.

Also would be great to have this a separate component. Is the plan to create a component at later time when we need to reuse this radio buttons?

ppr-ui/src/components/mhrRegistration/HomeLocation/HomeLandOwnership.vue Outdated Show resolved Hide resolved
ppr-ui/src/components/mhrRegistration/HomeLocation/HomeLandOwnership.vue Outdated Show resolved Hide resolved
ppr-ui/src/components/mhrRegistration/HomeLocation/HomeLandOwnership.vue Show resolved Hide resolved
ppr-ui/src/components/mhrRegistration/ReviewConfirm/HomeLocationReview.vue Outdated Show resolved Hide resolved
ppr-ui/src/composables/useRegistration.ts Show resolved Hide resolved
ppr-ui/tests/unit/HomeLandOwnership.spec.ts Outdated Show resolved Hide resolved
ppr-ui/src/components/mhrRegistration/ReviewConfirm/HomeLocationReview.vue Show resolved Hide resolved
ppr-ui/src/components/mhrRegistration/HomeLocation/HomeLandOwnership.vue Show resolved Hide resolved
ppr-ui/src/components/mhrRegistration/HomeLocation/HomeLandOwnership.vue Outdated Show resolved Hide resolved
@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://bcregistry-assets-dev--pr-1622-2u8ir4gq.web.app

@RuoxuanPengBC RuoxuanPengBC force-pushed the fix/update-yes-or-no-question-to-land-lease-or-own branch from cd0bd4a to 769c12a Compare November 20, 2023 20:58
@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://bcregistry-assets-dev--pr-1622-2u8ir4gq.web.app

1 similar comment
@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://bcregistry-assets-dev--pr-1622-2u8ir4gq.web.app

@RuoxuanPengBC
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://bcregistry-assets-dev--pr-1622-2u8ir4gq.web.app

@RuoxuanPengBC RuoxuanPengBC force-pushed the fix/update-yes-or-no-question-to-land-lease-or-own branch from 768008f to 7fbef02 Compare November 23, 2023 23:20
dimak1
dimak1 reviewed Nov 23, 2023
View reviewed changes
Copy link
Collaborator

@dimak1 dimak1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tagged you @RuoxuanPengBC in unresolved comments.

ppr-ui/src/composables/useRegistration.ts Show resolved Hide resolved
ppr-ui/src/components/mhrRegistration/HomeLocation/HomeLandOwnership.vue
height: 60px;
padding: 10px;
margin-right: 0px !important;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RuoxuanPengBC

dimak1
dimak1 approved these changes Nov 24, 2023
View reviewed changes
Copy link
Collaborator

@dimak1 dimak1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dimak1 dimak1 merged commit 786b23a into bcgov:main Nov 24, 2023
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cameron-eyds cameron-eyds cameron-eyds approved these changes

@dimak1 dimak1 dimak1 approved these changes

Assignees

@RuoxuanPengBC RuoxuanPengBC

Labels
MHR Registration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RuoxuanPengBC @bcregistry-sre @dimak1 @cameron-eyds