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

13021 - Special Resolution #499

Merged
merged 48 commits into from
May 24, 2023
Merged

13021 - Special Resolution #499

merged 48 commits into from
May 24, 2023

Conversation

seeker25
Copy link
Collaborator

@seeker25 seeker25 commented May 16, 2023

Issue #: bcgov/entity#13021

Description of changes:
Changes to support Special Resolution filing.

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

@seeker25
Copy link
Collaborator Author

Tech debt ticket: bcgov/entity#16489

@severinbeauvais
Copy link
Collaborator

Would be nice to get some other review approvals (otherwise I'm the code gatekeeper, and that's a team role).

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.

GTG (from my point of view) when you're happy with it.

@@ -1253,7 +1253,7 @@ export default class FilingTemplateMixin extends DateMixin {
name: documentsInfo?.certifiedRules?.name,
key: documentsInfo?.certifiedRules?.key,
url: entitySnapshot.businessDocuments?.documents?.certifiedRules,
previouslyInResolution: documentsInfo.certifiedRules?.includedInResolution,
previouslyInResolution: documentsInfo?.certifiedRules?.includedInResolution,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another option is to only set to store if you know the object is valid (otherwise you leave the store with its initial values), for example:

if (variable I care about exists) {
  set variable to store
}
// otherwise don't set to store

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! Lots and lots of learning for me here. Thanks for the opportunity to review 😄

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.

Lots of learning. LGTM! Thanks

@seeker25
Copy link
Collaborator Author

Just fixing the paper draft issue Severin pointed out

src/assets/styles/base.scss Outdated Show resolved Hide resolved
@seeker25
Copy link
Collaborator Author

seeker25 commented May 24, 2023

I've made some minor changes. I'm seeing another problem unrelated to this commit. The time it takes for npm run serve to run.. or npm run build seems to be fairly high. For example below an 8 minute build check (#499 this PR).

It was 4 minutes on this PR:
#497
https://github.com/bcgov/business-edit-ui/actions/runs/4867695121/jobs/8680505760

7 minutes:
#498
https://github.com/bcgov/business-edit-ui/actions/runs/5062176499/jobs/9087370052

@seeker25
Copy link
Collaborator Author

Merging, if you see any issues - can address it in another PR.

@seeker25 seeker25 merged commit 62a487a into bcgov:main May 24, 2023
@severinbeauvais
Copy link
Collaborator

I've made some minor changes. I'm seeing another problem unrelated to this commit. The time it takes for npm run serve to run.. or npm run build seems to be fairly high...

I wouldn't rely too much on GH build times, but I have noticed builds taking longer locally -- ever since the new packages, I think. Sometimes I think my WSL VM runs out of memory, too. I'll keep an eye out but I haven't found what it is, yet. (I wouldn't mind trying out Vite, though.)

@seeker25
Copy link
Collaborator Author

This PR has changes in it that freeze up the type checking:

#498

I've tried the PR before it and don't have the same issues:

#497

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