-
Notifications
You must be signed in to change notification settings - Fork 18
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
To-do Action - File Annual Report #17
Conversation
/gcbrun |
Temporary Url for review: https://business-dashboard-dev--pr-17-cc7zmt6k.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.
Looks good, few comments, few questions. Also, if you did not see, some cypress are failing.
|
||
<div v-if="displayItem===DisplayItem.None"> | ||
NONE NONE | ||
<div class="flex flex-col gap-0 w-full"> |
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.
Just for the record :P, I still think it would be better off if we had a component for each type of todo item.
**keep as is as we all agreed we can do it this way.
@@ -1,14 +1,20 @@ | |||
<template> | |||
<div | |||
class="flex flex-col gap-1.5 bg-gray-100" | |||
class="flex flex-col" |
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.
did gap not work ?
why I am asking is if we add any kind of margins inside container, or padding on the item, the border will only be up until those margins, will not go through entire container.
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 don't need to use gap here. An extra space will be added above each todo title except for the first one. To better illustrate this, I used gap-10
in the following screenshot.
The reason why I removed bg-gray-100
is that, when an to-do item is expanded, no top and bottom gap are added. (Alerts and Recent Filing History has such behavior but it is not the case in To-Do section)
* A filing's amalgamation application object from the API. See: | ||
* https://github.com/bcgov/business-schemas/blob/main/src/registry_schemas/schemas/amalgamation_application.json | ||
*/ | ||
export interface RegisteredRecordsAddressesI { |
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 just a question.
Are this and name request filing treated as amalgamation ? I see multiple interfaces added in different files.
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 actually don't know what that is haha. Just copied from the old codebase.
@@ -0,0 +1,76 @@ | |||
import { FilingTypes } from '@bcrs-shared-components/enums' |
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 good, keeping the ones we have in shared project.
@@ -213,6 +214,270 @@ export const useBcrosBusiness = defineStore('bcros/business', () => { | |||
return !!requestedFiling | |||
} | |||
|
|||
/** Check if the specified action is allowed, else False */ | |||
const isAllowed = (action: AllowableActionE): boolean => { |
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.
Is tis supposed to be in here or inside some util file ? I think we said we gona keep it in enumutils.ts ?
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 added a enum-utilities.ts
that contains some helper functions to compare filling status. (BTW, it is a EnumUtilities
class with static methods. But maybe we can just use functions instead of putting them inside a class)
For this isAllowed
function, it is a wrapper for isAllowedToFile()
so I think it is better to keep them in the same file.
src/stores/business.ts
Outdated
|
||
// TO-DO: the above line is commented out because we do not have 'BUSINESS_ID' in the sessionStorage | ||
// For now, we check if the currentBusiness exists in the business store. | ||
const isBusiness = !!useBcrosBusiness().currentBusiness.identifier |
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.
If we are keeping it here, I think you dont have to use Business store, as you are inside, am not sure what are consequences of that.
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.
Good catch! I will change that. Another update I need to make is, only one todo item can be expanded each time.
src/utils/todo.ts
Outdated
|
||
const enabled = task.enabled && !business.isDisableNonBenCorps() | ||
|
||
// TO-DO: business.isAllowed() will always return false for KIAL DEV 1 account |
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.
If not already created, please create a ticket for this issue so we look into it.
/gcbrun |
Temporary Url for review: https://business-dashboard-dev--pr-17-cc7zmt6k.web.app |
/gcbrun |
Temporary Url for review: https://business-dashboard-dev--pr-17-cc7zmt6k.web.app |
/gcbrun |
Temporary Url for review: https://business-dashboard-dev--pr-17-cc7zmt6k.web.app |
src/utils/enum-utilities.ts
Outdated
import { FilingTypes } from '@bcrs-shared-components/enums' | ||
|
||
// NB: These two methods are just for getPnedingCoa() in useBcrosFilings store | ||
// more methods to be added. Or the EnumUtilities class can be replaced by functions |
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 would update this comment to make these more generic if necessary (i.e. to pass in the desired filing type or filing status rather than have separate functions for every possibility). Also the name 'EnumUtilities' is a bit misleading I think - could we change that to something like 'FilingUtil' as this will always be specific to filings data I think?
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.
actually is there a reason this isn't in the filing store? It seems intuitive to have something like isFilingType(types: FilingTypes[]) / isStatus(status: FilingStatusE[]) in the filing store, similar to what I was asking for in the business store?
/gcbrun |
Temporary Url for review: https://business-dashboard-dev--pr-17-cc7zmt6k.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.
Nice work 👍
*Issue:*bcgov/entity#21350
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 namex license (Apache 2.0).