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.

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

14975-fix style and validation #457

Merged
merged 5 commits into from
Mar 17, 2023
Merged

Conversation

ketaki-deodhar
Copy link
Collaborator

@ketaki-deodhar ketaki-deodhar commented Mar 15, 2023

Issue #: /bcgov/entity#14975

Description of changes:
UX observations:

  1. display one check mark beside the related applicant type since only one type of applicant is required

image

  1. alignment of the action buttons be the same level as text on the left

image

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

@ketaki-deodhar ketaki-deodhar self-assigned this Mar 15, 2023
@@ -640,6 +640,7 @@ export default class ListPeopleAndRoles extends Mixins(CommonMixin, OrgPersonMix
.actions-width {
min-width: 150px;
max-width: 150px;
padding: 0.35rem !important;
}
Copy link
Collaborator Author

@ketaki-deodhar ketaki-deodhar Mar 15, 2023

Choose a reason for hiding this comment

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

This fix is for point#2 - to align action buttons with the text

image

<!-- <v-icon>mdi-circle-small</v-icon> -->
<v-icon v-if="isApplicantOrg" color="green darken-2" class="dir-valid">mdi-check</v-icon>
<v-icon v-else-if="!applicantOrgs" color="red">mdi-close</v-icon>
<v-icon v-else>mdi-circle-small</v-icon>
<span>A business or a corporation</span>
Copy link
Collaborator Author

@ketaki-deodhar ketaki-deodhar Mar 15, 2023

Choose a reason for hiding this comment

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

this is for point#1 - fix validations

Default Applicant:
image

Remove Person and add another Person
image

Remove Person and add Org
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

^^ Looks great! 😃

@ketaki-deodhar ketaki-deodhar marked this pull request as ready for review March 15, 2023 23:23
@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Merging #457 (4cc3dec) into main (eaf687f) will decrease coverage by 1.87%.
The diff coverage is 81.31%.

@@            Coverage Diff             @@
##             main     #457      +/-   ##
==========================================
- Coverage   85.70%   83.84%   -1.87%     
==========================================
  Files         177      185       +8     
  Lines        3289     3510     +221     
  Branches      524      711     +187     
==========================================
+ Hits         2819     2943     +124     
- Misses        468      566      +98     
+ Partials        2        1       -1     
Impacted Files Coverage Δ
...mponents/Alteration/Articles/CompanyProvisions.vue 100.00% <ø> (ø)
src/components/common/ErrorContact.vue 100.00% <ø> (ø)
...ourCompany/NameTranslations/AddNameTranslation.vue 100.00% <ø> (ø)
...nents/common/YourCompany/NameTranslations/index.ts 100.00% <ø> (ø)
src/dialogs/StaffPaymentErrorDialog.vue 100.00% <ø> (ø)
src/services/legal-services.ts 93.22% <0.00%> (-6.78%) ⬇️
src/utils/feature-flag-utils.ts 15.78% <ø> (ø)
src/mixins/filing-template-mixin.ts 39.00% <19.17%> (-5.08%) ⬇️
src/utils/FetchConfig.ts 95.12% <50.00%> (-2.32%) ⬇️
src/mixins/common-mixin.ts 82.95% <78.57%> (+6.21%) ⬆️
... and 80 more


@extend .actions-width;
margin-top: -8px;

Copy link
Collaborator Author

@ketaki-deodhar ketaki-deodhar Mar 16, 2023

Choose a reason for hiding this comment

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

Removed overall padding. margin-top at line 620 was being overridden by this (line 623). Reduced margin-top (line 620) to align with the column on left. It is misaligned in DEV as well. This fixes the alignment for other filings (Change, Correction and Conversion). I tested for all the other filing types

In DEV:
image

After Fix:
image

@severinbeauvais
Copy link
Collaborator

^^ I restarted the failed jobs -- looked like environment error, not code error.

Copy link
Collaborator

@severinbeauvais severinbeauvais left a comment

Choose a reason for hiding this comment

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

LGTM but please get another approval or 2 before merging.

@ketaki-deodhar
Copy link
Collaborator Author

^^ I restarted the failed jobs -- looked like environment error, not code error.

Thank you!

Copy link
Collaborator

@JazzarKarim JazzarKarim 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! Awesome changes 👍

Copy link
Contributor

@jonathan-longe jonathan-longe left a comment

Choose a reason for hiding this comment

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

Well done @ketaki-deodhar !

<v-icon v-else color="red">mdi-close</v-icon>
<!-- <v-icon>mdi-circle-small</v-icon> -->
<v-icon v-if="isApplicantPerson && hasApplicant" color="green darken-2" class="dir-valid">mdi-check</v-icon>
<v-icon v-else-if="!isApplicantOrg || !hasApplicant" color="red">mdi-close</v-icon>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shows correct validation if more than one applicants in the List.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Screenshot please :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Beautiful :)

@ketaki-deodhar ketaki-deodhar merged commit 1c3db81 into bcgov:main Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants