Add missing information to object details screens #35
Conversation
kamorel
changed the title
Coverage Report (Frontend)
|
Coverage Report (Application)
|
| @@ -2,25 +2,42 @@ | |||
| import { onMounted, ref } from 'vue'; | |||
| import { useRoute } from 'vue-router'; | |||
| import { storeToRefs } from 'pinia'; | |||
| import Button from 'primevue/button'; | |||
There was a problem hiding this comment.
Style: We'll want to reorder the imports so that the pinia one comes before the vue one.
| import { onMounted, ref, watch } from 'vue'; | ||
| import { storeToRefs } from 'pinia'; | ||
| import { useBucketStore, useUserStore } from '@/store'; | ||
| import GridRow from '@/components/form/GridRow.vue'; | ||
| import { RouteNames } from '@/utils/constants'; |
There was a problem hiding this comment.
Style: Import ordering should be done based on library name. Newline between external and internal deps.
import { storeToRefs } from 'pinia';
import { onMounted, ref, watch } from 'vue';
import GridRow from '@/components/form/GridRow.vue';
import { useBucketStore, useUserStore } from '@/store';
import { RouteNames } from '@/utils/constants';| watch( props, async () => { | ||
| await getBucketData(); | ||
| await getUserData(); | ||
| }); |
There was a problem hiding this comment.
Async/await: Is there an actual linear dependency where getUserData needs to wait on getBucketData to complete first? If not, we should let these both fire asynchronously. That means the following is sufficient:
watch(props, () => {
getBucketData();
getUserData();
});
There was a problem hiding this comment.
What's the need for the watches in these components. It's a load->loading->display pattern right? Or are there refreshes somewhere or subscriptions that would update that data?
There was a problem hiding this comment.
For the sidebar in the object list, the first time it is opened it gets mounted but if you click the info icon again on a different object it doesn't remount, it just receives new props.
| // State | ||
| import { useObjectStore } from '@/store'; | ||
| // Component | ||
|
|
There was a problem hiding this comment.
Style: We should try to reorder the primevue/* imports to come before the vue ones now that we're removing the comment groupings on them unless that causes a compilation behavior due to import ordering.
| v-if="value && link" | ||
| class="col-9" | ||
| > | ||
| <router-link |
There was a problem hiding this comment.
i like the way you made GriDRow re-usable.
you might be able to make this more compact:
<div v-if="value && label" class="grid-row">
<div class="col-3">{{ label }}</div>
<div class="col-9">
<router-link v-if="link" :to="link">{{ value }}</router-link>
<span v-else>{{value }}</span>
</div>
</div>There was a problem hiding this comment.
I agree with you here, but unfortunately nested div's break the formatting for some reason I wasn't able to figure out.
| const permissionsObjectId = ref(''); | ||
| const permissionsObjectName = ref(''); | ||
|
|
||
| const getObjectInfo = (objId: string) => { |
There was a problem hiding this comment.
not suggesting we do it now, by personally i would find some code comments useful. or jsdoc or whatever.
// gets details, perms for a single object from objectList state in objectStore.
the toast summary on line 45 could be changed to 'unable to load object (singular)'
There was a problem hiding this comment.
separate note. Does this component call the objectStore.listObjects(objId) method.. in case someone navigates straight to this page. Rather than just checking the existing objectList.value.
maybe Luke was going to take a look at this.. or it;s for another ticket
There was a problem hiding this comment.
I have a fix for that almost to go, but this ticket will merge conflict the heck out of that
https://apps.nrs.gov.bc.ca/int/jira/browse/SHOWCASE-3049
So would leave this and once this is merged in I'll sort it for 3049
| v-if="isInfoLoaded" | ||
| class="pl-2" | ||
| > | ||
| <ObjectProperties :object-properties="objectInfo" /> |
There was a problem hiding this comment.
you could rename these props to object-info="objectInfo" so that they are received in the child component as
objectInfo
kamorel
force-pushed
the
feature/obj-properties-updates
branch
from
9882833 to
2ed4bae
Compare
Description
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist
Further comments