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

16636 Update Extrapro NR flow (continued) #655

Merged

Conversation

severinbeauvais
Copy link
Collaborator

@severinbeauvais severinbeauvais commented Aug 18, 2023

Issue #: bcgov/entity#16636

Description of changes:
- app version = 5.0.25
- moved Bullets Colin Link into Search
- hide details in misc components
- renamed getIsConversion -> isConversion
- refactored layout in Search
- added Federal bullets
- moved isXXX computed to getters.ts
- updated misc computed
- title-cased entity names (per uxpin)
- updated search test suite

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This component, with slots for the previous code, was a good idea at the time, but with the new way of navigating it was becoming "messy" to have knowledge of the flows in the parent (Search) and here, so I moved what was in here back into the parent.

As we near the end of the flow changes, there may be an opportunity(ies) to break out some code into sub-components, but I see nothing obvious right now that wouldn't increase, instead of decrease, complexity.

One thing to watch for is to keep row/col layout in a single file. For example, I think we shouldn't have row declarations in Search and columns in sub-components. Instead, all rows/cols should be in Search, and the sub-components can be the column contents.

Copy link
Collaborator

@JazzarKarim JazzarKarim Aug 21, 2023

Choose a reason for hiding this comment

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

I see. Makes so much sense Sev. Having it with slots previously provided limitations on how flexible we could've been with the flows. Cool!

@@ -2,6 +2,7 @@
<v-select
class="request-action-select"
filled
hide-details="auto"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This keeps the component "short" unless some detail messages (errors) need to be shown. This is to prevent excess bottom whitespace, which we sometimes absorb with negative bottom margins -- but this is better.

@@ -18,7 +18,7 @@
<v-col v-if="showDesignationSelect" :class="{'pl-3': !isMobile}" cols="12" lg="3">
<v-select filled
:items="designationOptions"
label="Enter designation"
label="Select a Designation"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be consistent with Search page (as per uxpin).

@@ -3,7 +3,7 @@ import { EntityI } from '@/interfaces/models'

export const EntityTypesBcData: EntityI[] = [
{
text: 'Sole proprietorship',
text: 'Sole Proprietorship',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The uxpin showed some entity types in title case, so I changed them all over.

- moved Bullets Colin Link into Search
- hide details in misc components
- renamed getIsConversion -> isConversion
- refactored layout in Search
- added Federal bullets
- moved isXXX computed to getters.ts
- updated misc computed
- title-cased entity names (per uxpin)
- updated search test suite
<v-col cols="12" class="pt-0 font-weight-bold h6">
<v-container fluid id="search-container" class="copy-normal pt-10 px-10 pb-12">
<v-row>
<v-col cols="12" class="font-weight-bold h6">
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


<v-row class="mt-6">
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 got rid of the "second row". Most sections here are simply columns, which automatically wrap or stack depending on screen width. Eg,

image

content-class="top-tooltip"
transition="fade-transition"
:disabled="isMobile"
<!-- Type of Business, only when an entity type is selected (or federal) -->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved here from previous sub-component.

return false
}

get showBusinessLookup () {
if (this.isChangeName) return true
if (this.isAlteration) return true
if (this.isConversion) return true
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A conversion is an alteration, but the NR Request Action Code is CNV so I renamed this getter accordingly.

@severinbeauvais
Copy link
Collaborator Author

/gcbrun

@pwei1018
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-655-fw1o16m2.web.app

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.

LGTM! 👍

Copy link
Collaborator

@kzdev420 kzdev420 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.

much cleaner! LGTM 👍

@severinbeauvais severinbeauvais merged commit 26361c9 into bcgov:feature-way-of-navigating Aug 21, 2023
5 checks passed
JazzarKarim pushed a commit to JazzarKarim/namerequest that referenced this pull request Aug 24, 2023
- moved Bullets Colin Link into Search
- hide details in misc components
- renamed getIsConversion -> isConversion
- refactored layout in Search
- added Federal bullets
- moved isXXX computed to getters.ts
- updated misc computed
- title-cased entity names (per uxpin)
- updated search test suite
JazzarKarim pushed a commit to JazzarKarim/namerequest that referenced this pull request Aug 24, 2023
- moved Bullets Colin Link into Search
- hide details in misc components
- renamed getIsConversion -> isConversion
- refactored layout in Search
- added Federal bullets
- moved isXXX computed to getters.ts
- updated misc computed
- title-cased entity names (per uxpin)
- updated search test suite
severinbeauvais added a commit to severinbeauvais/namerequest that referenced this pull request Aug 29, 2023
- moved Bullets Colin Link into Search
- hide details in misc components
- renamed getIsConversion -> isConversion
- refactored layout in Search
- added Federal bullets
- moved isXXX computed to getters.ts
- updated misc computed
- title-cased entity names (per uxpin)
- updated search test suite
JazzarKarim pushed a commit to JazzarKarim/namerequest that referenced this pull request Sep 8, 2023
- moved Bullets Colin Link into Search
- hide details in misc components
- renamed getIsConversion -> isConversion
- refactored layout in Search
- added Federal bullets
- moved isXXX computed to getters.ts
- updated misc computed
- title-cased entity names (per uxpin)
- updated search test suite
JazzarKarim pushed a commit that referenced this pull request Sep 11, 2023
- moved Bullets Colin Link into Search
- hide details in misc components
- renamed getIsConversion -> isConversion
- refactored layout in Search
- added Federal bullets
- moved isXXX computed to getters.ts
- updated misc computed
- title-cased entity names (per uxpin)
- updated search test suite
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.

5 participants