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

17468 Refactored search.vue + implemented xpro aml flow #674

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

severinbeauvais
Copy link
Collaborator

@severinbeauvais severinbeauvais commented Sep 7, 2023

Issue #: bcgov/entity#17468

This is incomplete code! What I have in mind is merging to this to a feature branch where several of us can rebuild the flows using the new architecture. And we can compare these flows to what's currently in Dev.

Description of changes:
- app version = 5.1.0
- implemented sub-components for search.vue blocks
- refactored search.vue to separate flows using sub-components (STILL WIP)
- fixed tab title size on small screens
- added search variables to store
- created search-mixin.ts for common getters, setters, etc.
- misc cleanup

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the namerequest license (Apache 2.0).

@severinbeauvais severinbeauvais self-assigned this Sep 7, 2023
@severinbeauvais severinbeauvais changed the base branch from main to feature-way-of-navigating September 7, 2023 21:18
@severinbeauvais severinbeauvais force-pushed the 17468 branch 5 times, most recently from d3f4a0f to 0d97c55 Compare September 11, 2023 19:43
@severinbeauvais severinbeauvais changed the base branch from feature-way-of-navigating to main September 11, 2023 19:43
@JazzarKarim
Copy link
Collaborator

Sev, I'll stop doing any work in this UI after merging my latest small PR.

@severinbeauvais severinbeauvais changed the title 17468 WIP 17468 Refactored search.vue + implemented xpro aml flow Sep 12, 2023
@severinbeauvais severinbeauvais force-pushed the 17468 branch 4 times, most recently from 40c2d08 to 09f8a02 Compare September 12, 2023 23:13
id="name-input-component"
:is-mras-search="!isNameSearch"
/>
<NameInput :is-mras-search="!isNameSearch" />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

id is already set in the component.

@@ -89,7 +89,6 @@ export default class Tabs extends Mixins(CommonMixin) {

.mobile-font {
font-size: $px-14;
max-width: 120px;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fix for tab title:

image

companyType: CompanyTypes
jurisdiction: any
request: RequestActionsI
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I struggled for a while with reactivity on variables in the search mixin and the components sometimes not seeing the changes. Ultimately, it looks like there can only be 1 watcher or reactive statement for them. So, I moved the variables to the store instead, and now reactivity works as expected: everyone can see all changes,

@@ -16,6 +16,12 @@
</keep-alive>
</transition>
</div>

<!-- FOR DEBUGGING -->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Once again, I'll delete this and the block below before my final commit.

@severinbeauvais severinbeauvais force-pushed the 17468 branch 4 times, most recently from 95f9734 to c06653b Compare September 13, 2023 15:39
@severinbeauvais severinbeauvais changed the base branch from main to feature-search-refactor September 13, 2023 15:39
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm doing some more cleanup in here before we merge this PR (to make it easier for several people to work in here at the same time).

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.

I get now the direction you want to push the code towards. The code already looks much better and easier to work in / understand.

@severinbeauvais severinbeauvais marked this pull request as ready for review September 13, 2023 16:37
@@ -0,0 +1,52 @@
<template>
<div id="corp-number-checkbox">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: this component is NOT a v-col like the others.

<template>
<v-col id="entity-type" :cols="cols" :md="md">
<v-text-field
v-if="isConversion && isBenBusiness"
Copy link
Collaborator Author

@severinbeauvais severinbeauvais Sep 13, 2023

Choose a reason for hiding this comment

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

This logic should be moved to the parent.

Correction -- this logic belongs here as it's a special case for BEN conversions. Otherwise the code block below is used. This is probably a temporary situation (only some alterations are supported atm).

@@ -0,0 +1,54 @@
<template>
<v-col id="numbered-company-bullets" :cols="cols" :md="md">
<template v-if="isConversion && !isAlterOnline(getConversionType)">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's another special case, where this component always displays but, depending on the conditionals, some code block or another is displayed.

My point is that logic on whether to display a component belongs in the parent, but within a component there can be some special cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, big changes here.

  1. most code blocks are now sub-components
  2. there is a search-mixin that contains common imports and getters, etc
  3. some components still contain some logic that is specific to them only
  4. some of the common search variables are now in the store (for reactivity)
  5. all flows are now separate!
  6. there is a bit of common code at the top of the template (request action menu) and also at the bottom (buttons)
  7. logic for displaying components should be in here, not in the sub-component (except for some special cases)

- implemented sub-components for search.vue blocks
- refactored search.vue to separate flows using sub-components (STILL WIP)
- fixed tab title size on small screens
- added search variables to store
- created search-mixin.ts for common getters, setters, etc.
- misc cleanup
@severinbeauvais
Copy link
Collaborator Author

There's no point in creating a temp URL for this as we're all going to run it locally shortly :)

Once this is merged, I'll set up a quick meeting to walk you through the changes and how I expect the new code to look.

@JazzarKarim
Copy link
Collaborator

There's no point in creating a temp URL for this as we're all going to run it locally shortly :)

Once this is merged, I'll set up a quick meeting to walk you through the changes and how I expect the new code to look.

Sounds good!

Copy link
Collaborator

@leodube-aot leodube-aot left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@ketaki-deodhar ketaki-deodhar left a comment

Choose a reason for hiding this comment

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

👍

@severinbeauvais severinbeauvais merged commit 4d9fa0e into bcgov:feature-search-refactor Sep 13, 2023
4 of 5 checks passed
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