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

21977 Got rid of Enum Mixin #681

Merged
merged 2 commits into from
Jul 15, 2024
Merged

Conversation

severinbeauvais
Copy link
Collaborator

@severinbeauvais severinbeauvais commented Jul 15, 2024

Issue #: bcgov/entity#21977

This is a cleanup activity for the subject ticket. This change should not affect functionality at all.

Description of changes:

  • app version = 7.3.12
  • replaced all calls to Enum Mixin methods by calls to Enum Utilities class
  • deleted enum-mixin.ts
  • added initial PendingList.vue (WIP but disabled)
  • replaced some getEntityStatus with store isXXX helpers

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

- replaced all calls to Enum Mixin methods by calls to Enum Utilities class
- deleted enum-mixin.ts
- added initial PendingList.vue (WIP but disabled)
- replaced some getEntityStatus with store isXXX helpers
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 new component isn't being used yet. Right now it's basically a copy of TodoList.vue. I recommend you don't bother reviewing this component as it will change significantly. I'm working on its data architecture presently. I hope this component will be "simple" like FilingHistoryList instead of really complex like TodoList.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In some cases, mixins can be very practical, but they "pollute" the component namespace (ie, all their methods are attached to this). This hasn't been a problem (due to long naming) but it's usually not a good practise.

Something I thought was a problem was not being able to (easily) find the definitions/declarations for mixin methods. By now having them in the EnumUtilities class, it's obvious where they are and you can F12 to them.

We started deprecating this mixin in early 2023 but did not finish getting rid of it. I'm getting rid of it now because I'm going to restructure a few things (states and getters).

@@ -96,6 +96,46 @@ export default class EnumUtilities {
return (item.name === FilingTypes.AGM_LOCATION_CHANGE)
}

/** Returns True if filing is an Amalgamation Application. */
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These were simply moved up from below so the methods are (mostly) in alphabetized.

@@ -44,6 +44,23 @@
/>
</section>

<!-- Pending section-->
<section
v-if="false"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New section, in progress in the subject ticket, not displayed at the moment.

@@ -83,6 +83,7 @@ describe('Dashboard - UI', () => {
expect(wrapper.findComponent(DirectorListSm).exists()).toBe(true)
})

// *** TODO: add tests for Pending status
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 will be doing this in the subject ticket, in future commits.

- renamed businessStore::setStateFiling() -> setStateFilingUrl() to reduce confusion with rootStore::setStateFiling()
@severinbeauvais
Copy link
Collaborator Author

/gcbrun

Copy link
Collaborator

@ArwenQin ArwenQin left a comment

Choose a reason for hiding this comment

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

LGTM!

@severinbeauvais severinbeauvais merged commit 831d616 into bcgov:main Jul 15, 2024
5 checks passed
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.

Did not finish review before it was merged. Looked good though!

@severinbeauvais
Copy link
Collaborator Author

Did not finish review before it was merged. Looked good though!

That's OK. I wanted to merge this asap so others could build upon it if needed (and so I could rebase and keep each PR concise).

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.

3 participants