#1866 - Remove props any from the vue application #1916
Conversation
andrepestana-aot
requested review from
ann-aot,
dheepak-aot,
sh16011993,
andrewsignori-aot,
guru-aot and
JasonCTang
| const { getLocationName } = useInstitutionState(); | ||
| const tab = ref("coe-tab"); | ||
|
|
||
| const locationName = computed(() => { | ||
| return getLocationName(parseInt(props.locationId)); | ||
| return getLocationName(props.locationId); |
sources/packages/web/src/components/common/ScholasticStandingForm.vue
Outdated
Show resolved
Hide resolved
| @@ -138,7 +142,11 @@ export const institutionRoutes: Array<RouteRecordRaw> = [ | |||
| default: ActiveApplicationsSummary, | |||
| sidebar: InstitutionHomeSideBar, | |||
| }, | |||
| props: true, | |||
| props: { | |||
| default: (route: RouteLocationNormalized) => ({ | |||
There was a problem hiding this comment.
What is the benefit of having the default here?
Would the below not be enough?
props: (route: RouteLocationNormalized) => ({
locationId: parseInt(route.params.locationId[0])
}),
},There was a problem hiding this comment.
I think the default adds props to the default component,
and if there are any param for the sidebar component then props: { sidebar: (route: RouteLocationNormalized) => ({ can also be added. correct me if I am wrong @andrepestana-aot
There was a problem hiding this comment.
I tried doing that but it didn't accept:
There was a problem hiding this comment.
I see the point but maybe we are doing something else wrong.
The function mode seems to be available and we maybe are missing something about how to have it properly typed.
Either way, I would be ok to proceed as it is right now 😉
There was a problem hiding this comment.
There was a problem hiding this comment.
I think the default adds props to the default component,
props: { sidebar: (route: RouteLocationNormalized) => ({can also be added. correct me if I am wrong @andrepestana-aot
@ann-aot After our chat I got the point here and I am ok about doing the parse for the individual components. As we talk, for an upcoming PR, where we would be doing the changes globally, I would like to explore some centralized method to avoid repeating code like route.params.locationId[0].
@andrepestana-aot since this was not the main focus of the PR I am ok as it is right now.
There was a problem hiding this comment.
I think the default adds props to the default component,
props: { sidebar: (route: RouteLocationNormalized) => ({can also be added. correct me if I am wrong @andrepestana-aot
Yes, that's right.
@ann-aot After our chat I got the point here and I am ok about doing the parse for the individual components. As we talk, for an upcoming PR, where we would be doing the changes globally, I would like to explore some centralized method to avoid repeating code like
route.params.locationId[0]. @andrepestana-aot since this was not the main focus of the PR I am ok as it is right now.
Ok. @andrewsignori-aot I didn't understand well what you mean about a centralized method. Do mean creating a name function to be reused for the named objects?
| @@ -34,7 +34,7 @@ export default defineComponent({ | |||
| //TODO: This emit needs to be removed when the program and offering header component | |||
| //TODO: is enhanced to load header values with it's own API call. | |||
| emits: ["getHeaderDetails"], | |||
| setup(props: any, context: SetupContext) { | |||
| setup(props, context: SetupContext) { | |||
There was a problem hiding this comment.
Like props, context doesn't need to be explicitly typed.
| @@ -35,7 +35,7 @@ export default defineComponent({ | |||
| default: false, | |||
| }, | |||
| }, | |||
| setup(_props: any, context: SetupContext) { | |||
| setup(_props, context: SetupContext) { | |||
There was a problem hiding this comment.
| @@ -104,7 +104,7 @@ export default defineComponent({ | |||
| }, | |||
| }, | |||
| emits: ["submitData"], | |||
| setup(props: any, context: any) { | |||
| setup(_props, context: any) { | |||
There was a problem hiding this comment.
Context is typed to any, please remove the explicit typing.
https://github.com/bcgov/SIMS/pull/1916/files#r1184391057
|
Good work @andrepestana-aot . Added one comment which is applicable to many files. |
There was a problem hiding this comment.
It looks great. I am giving my approval considering the comments from @ann-aot and @dheepak-aot will be addressed.
|
Kudos, SonarCloud Quality Gate passed!
|
Backend Unit Tests Coverage Report
|
E2E Queue Consumers Coverage Report
|
E2E Workflow Workers Coverage Report
|
E2E SIMS API Coverage Report
|
any;