Options API to Composition API #403
Conversation
|
SonarCloud Quality Gate failed.
|
severinbeauvais
requested review from
kialj876,
thorwolpert,
cameron-eyds and
pwei1018
| @@ -41,11 +41,12 @@ | |||
| "v-mask": "^2.3.0", | |||
| "vue": "^2.7.10", | |||
| "vue-affix": "^0.5.2", | |||
| "vue-router": "^3.6.5", | |||
| "vue-router": "^4.1.5", | |||
| "vue-template-compiler": "^2.7.10", | |||
There was a problem hiding this comment.
vue-template-compiler is a dev dependency.
| } | ||
| const { formatYyyyMmDd } = DateComposable() | ||
| const { isBComp } = AllowableActionsComposable() // probably can pull from state getters | ||
| const { getCurrentDate } = FilingComposable() |
There was a problem hiding this comment.
getCurrentDate come from Vuex getters.
There was a problem hiding this comment.
Yes it does, I only used the getCurrentDate from FilingComposable because I hadn't implemented the getCurrentDate in the Vuex getters and I wanted a complete component
There was a problem hiding this comment.
Ok.
It might be good to be concise moving forward.
|
|
||
| @Component({}) | ||
| export default class ArDate extends Mixins(DateMixin) { | ||
| @State nextARDate!: string |
There was a problem hiding this comment.
Where does this come from?
There was a problem hiding this comment.
I didn't implement this. I am not sure of the best way to implement state it seems like one of the advantages of the composition api is the ability to share states between different components. So I am not really sure what the best way is to handle @state
There was a problem hiding this comment.
Accessing state directly was a cheat, left over from the initial implementation of this UI. It's much better to use getters.
There was a problem hiding this comment.
Oh okay! This could possibly be a good refactor to do before changing API's I am making some tickets for that. I can add this.
| import { Component } from 'vue-property-decorator' | ||
|
|
||
| @Component({}) | ||
| export default class ContactInfo extends Vue {} |
There was a problem hiding this comment.
How does this component get named?
There was a problem hiding this comment.
The component is named as a variable when it is imported.
i.e. import ContactInfo from './ContactInfo.vue <ContactInfo /> works for this component and
import SomeRandomName from './ContactInfo.vue' <SomeRandomName /> works exactly the same
There was a problem hiding this comment.
So then we can't do import * from <whatever> ?
There was a problem hiding this comment.
I think you do would do import * as All from ./components <All.Foo /> <All.Bar /> There may be a more elegant solution but I haven't found one yet
| const isFirm = computed(() => store.getters['isFirm'] as boolean) | ||
| const isCoop = computed(() => store.getters['isCoop'] as boolean) | ||
| const hasBlockerExceptStaffApproval = computed(() => store.getters['hasBlockerExceptStaffApproval'] as boolean) | ||
| const isGoodStanding = computed(() => store.getters[''] as boolean) |
There was a problem hiding this comment.
This is really repetitive. Is there a way to combine these and just import/declare a single thing?
There was a problem hiding this comment.
Or we could write a helper?
Or is there a decorator for this somewhere?
There was a problem hiding this comment.
Yes this is way to repetitive. This is what I mean when I think we would need to do a major refactor. As code used anywhere can be reactive this could be replaced with
const { getEntityType, hasBlcker, ect.... } = getters where all the computed methods are stored
| * @param keys the nested object keys which to omit properties from | ||
| * @param prop the properties to be removed | ||
| */ | ||
| const omitProps = (baseObj: any, keys: Array<string>, prop: Array<string>): any => { |
There was a problem hiding this comment.
Please set type of baseObj to object.
| * Ref: https://www.npmjs.com/package/serverdate | ||
| * @returns a promise to return a Date object | ||
| */ | ||
| const getServerDate = async (): Promise<Date> => { |
There was a problem hiding this comment.
Do we have to use this function format or can we do this?
async function getServerDate (): Promise<Date> {
...
}
I think this ^^^ is easier to read. What do you think?
There was a problem hiding this comment.
The function format is better. I agree.
| return { | ||
| fieldSorter | ||
| } | ||
| } |
There was a problem hiding this comment.
Do you think this is better as a composable or would it be better as a standalone util function like these?
https://github.com/bcgov/business-edit-ui/tree/main/src/utils
There was a problem hiding this comment.
I am not sure. It very well might be. Right now what I have converted over is the composition api with a structure of an options api app. Which I don't think uses the composition api to its full potential
| routes, | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| scrollBehavior (to, from, savedPosition) { | ||
| // see https://router.vuejs.org/guide/advanced/scroll-behavior.html | ||
| return { x: 0, y: 0 } | ||
| return { left: 0, top: 0 } |
There was a problem hiding this comment.
Wow, you've done a ton of R&D to figure all this out. Nice work! 👍
| import { Debounce } from 'vue-debounce-decorator' | ||
| <script setup lang="ts"> | ||
| import _ from 'lodash' | ||
| import { computed, ref, watch, defineEmits } from 'vue' |
There was a problem hiding this comment.
Does defineProps need to be imported here as well?
There was a problem hiding this comment.
No and defineEmits doesn't need to be there either. Both are part of script setup
Issue #: /bcgov/entity#13675
Description of changes:
The work done here isn't complete or completely correct it still needs to be tested.
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).