Refactor File Details page #177
Conversation
Coverage Report (Application)
|
TimCsaky
force-pushed
the
computed
branch
5 times, most recently
from
d0759b8 to
40e6dbe
Compare
TimCsaky
force-pushed
the
object-table
branch
3 times, most recently
from
82359bc to
edb6909
Compare
TimCsaky
force-pushed
the
computed
branch
2 times, most recently
from
2456a7f to
f165231
Compare
Coverage Report (Frontend)
|
TimCsaky
requested review from
jujaga,
kyle1morel,
wilwong89,
jatindersingh93 and
norrisng-bc
as code owners
move and conolidate async action calls for data convert some store 'find' actions into getters reduce watch pattern
use models to to pass current and latest versionId's extend action to fetchMetadata/tags for all versions in one shot
| version.value = versionStore.findVersionById(props.versionId); | ||
| onMounted(async () => { | ||
| const head = await objectStore.headObject(props.objectId); | ||
| const isPublic = head?.status === 204; |
There was a problem hiding this comment.
As this HTTP 204 check is only being used in one place, it might be better to just inline this on line 88.
| // Actions | ||
| const router = useRouter(); | ||
| const versionId = defineModel<string>('versionId'); | ||
| const versions: Ref<Array<Version>> = computed((): any => getVersionsByObjectId.value(props.objectId)); |
There was a problem hiding this comment.
Should that any be Array<Version>? Looks like the any is describing the return structure type if I'm reading this right.
| const router = useRouter(); | ||
| const versionId = defineModel<string>('versionId'); | ||
| const versions: Ref<Array<Version>> = computed((): any => getVersionsByObjectId.value(props.objectId)); | ||
| const tableData: Ref<Array<VersionDataSource>> = computed(() => { |
There was a problem hiding this comment.
Might be this for more strict ts typing?
Ref<Array<VersionDataSource>> = computed((): Array<VersionDataSource> => {| watch(props, async () => { | ||
| await userStore.fetchUsers({ userId: versions.value.map((x: Version) => x.createdBy) }); |
There was a problem hiding this comment.
Not completely certain if we really need the async and await here as this callback just fires and finishes when it finishes. There doesn't appear to be any dependent functions that need to be blocking on this specific thread.
frontend/src/store/userStore.ts
Outdated
| () => (userId: string | undefined) => state.userSearch.value.find((x: User) => userId === x.userId) | ||
| ), | ||
| // returns an array of Users corresponding to provided user IDs | ||
| getUsers: computed(() => (userIds: Array<string> | undefined) => { |
There was a problem hiding this comment.
Not absolutely certain, but assuming no typescript snafus, it might be better to have the blah | undefined be marked as blah? instead to signal optionality?
There was a problem hiding this comment.
I'm presuming this is an intentional change to the file details page where no matter what, we want routing to always reroute a url to go from /details?objectId=blah to /details?objectId=blah&versionId=foo in every case? We want to make sure this url isn't too hard-coerced and can still forward-hydrate as this will be the main entrypoint for numerous user flows.
There was a problem hiding this comment.
visiting the file details page with a url that includes the versionId is is still supported, the page does load with that version.
|
Temporarily closing and reopening due to pipeline issues |
Description
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist
Further comments