Skip to content

Commit

Permalink
Simplify media FetchState and fix fetching errors (#5323)
Browse files Browse the repository at this point in the history
* Refactor FetchState to use discriminated union

Fixes #5322

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/WordPress/openverse/issues/5322?shareId=XXXX-XXXX-XXXX-XXXX).

* Fix loading labels, results, no result on later pages

* Make `canLoadMore` external prop

* Fix collection header stories

* Refactor i18n search result count labels

* Improve asyncData error in search.vue
  • Loading branch information
obulat authored Jan 16, 2025
1 parent 56be647 commit 6b87911
Show file tree
Hide file tree
Showing 31 changed files with 578 additions and 565 deletions.
1 change: 1 addition & 0 deletions frontend/nuxt.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export default defineNuxtConfig({
port: 8443,
host: "0.0.0.0",
},
devtools: { enabled: true },
imports: {
autoImport: false,
},
Expand Down
9 changes: 3 additions & 6 deletions frontend/shared/types/fetch-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ export interface FetchingError {
details?: Record<string, string>
}

export interface FetchState {
isFetching: boolean
hasStarted?: boolean
isFinished?: boolean
fetchingError: FetchingError | null
}
export type FetchState =
| { status: "idle" | "fetching" | "success"; error: null }
| { status: "error"; error: FetchingError }
10 changes: 3 additions & 7 deletions frontend/src/components/VCollectionHeader/VCollectionHeader.vue
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,12 @@ const showCollectionExternalLink = computed(() => {
return Boolean(props.collectionParams.collection !== "tag" && url.value)
})
const { getI18nCollectionResultCountLabel } = useI18nResultsCount()
const showLoading = computed(() => mediaStore.showLoading)
const { getI18nCollectionResultCountLabel } = useI18nResultsCount(showLoading)
const resultsLabel = computed(() => {
if (mediaStore.resultCount === 0 && mediaStore.fetchState.isFetching) {
return ""
}
const resultsCount = mediaStore.results[props.mediaType].count
return getI18nCollectionResultCountLabel(
resultsCount,
mediaStore.results[props.mediaType].count,
props.mediaType,
props.collectionParams.collection
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ export const AllCollections: Omit<Story, "args"> = {
const mediaStore = useMediaStore()
mediaStore.$patch({
results: { image: { count: 240 } },
mediaFetchState: {
image: { status: "success", error: null },
audio: { status: "success", error: null },
},
})
return () =>
h(
Expand Down
28 changes: 7 additions & 21 deletions frontend/src/components/VContentLink/VContentLink.vue
Original file line number Diff line number Diff line change
@@ -1,43 +1,29 @@
<script setup lang="ts">
import { useNuxtApp } from "#imports"
import { computed } from "vue"
import type { SupportedMediaType } from "#shared/constants/media"
import useSearchType from "~/composables/use-search-type"
import { useHydrating } from "~/composables/use-hydrating"
import { useI18nResultsCount } from "~/composables/use-i18n-utilities"
import VButton from "~/components/VButton.vue"
import VIcon from "~/components/VIcon/VIcon.vue"
const props = defineProps<{
mediaType: SupportedMediaType
/**
* Current search term for aria-label.
*/
searchTerm: string
/**
* The number of results that the search returned. The link
* will be disabled if this value is zero.
*/
resultsCount: number
/**
* The route target of the link.
*/
to?: string
to: string | undefined
labels: {
aria: string
visible: string
}
}>()
defineEmits<{
"shift-tab": [KeyboardEvent]
}>()
const { getI18nCount, getI18nContentLinkLabel } = useI18nResultsCount()
const resultsCountLabel = computed(() => getI18nCount(props.resultsCount))
const resultsAriaLabel = computed(() =>
getI18nContentLinkLabel(props.resultsCount, props.searchTerm, props.mediaType)
)
const { activeType } = useSearchType({ component: "VContentLink" })
const { $sendCustomEvent } = useNuxtApp()
Expand All @@ -56,7 +42,7 @@ const { doneHydrating } = useHydrating()
<VButton
as="VLink"
:href="to"
:aria-label="resultsAriaLabel"
:aria-label="labels.aria"
variant="bordered-gray"
size="disabled"
:disabled="!doneHydrating"
Expand All @@ -70,7 +56,7 @@ const { doneHydrating } = useHydrating()
</p>
<span
class="label-regular sm:description-regular text-secondary group-hover/button:text-default sm:ms-auto"
>{{ resultsCountLabel }}</span
>{{ labels.visible }}</span
>
</VButton>
</template>
2 changes: 1 addition & 1 deletion frontend/src/components/VHeader/VHeaderDesktop.vue
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const uiStore = useUiStore()
const isSidebarVisible = inject<Ref<boolean>>(IsSidebarVisibleKey)
const isFetching = computed(() => mediaStore.fetchState.isFetching)
const isFetching = computed(() => mediaStore.isFetching)
const { $sendCustomEvent } = useNuxtApp()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ const isInputFocused = ref(false)
const contentSettingsOpen = ref(false)
const appliedFilterCount = computed(() => searchStore.appliedFilterCount)
const isFetching = computed(() => mediaStore.fetchState.isFetching)
const isFetching = computed(() => mediaStore.isFetching)
/**
* The selection range of the input field. Used to make sure that the cursor
Expand Down
18 changes: 4 additions & 14 deletions frontend/src/components/VLoadMore.vue
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ import { useSearchStore } from "~/stores/search"
import VButton from "~/components/VButton.vue"
const props = defineProps<
defineProps<
SingleResultProps & {
searchType: SupportedSearchType
isFetching: boolean
canLoadMore: boolean
}
>()
Expand All @@ -39,13 +39,7 @@ const eventPayload = computed(() => {
}
})
/**
* Whether we should show the "Load more" button.
* If the fetching for the current query has started, there is at least
* 1 page of results, there has been no fetching error, and there are
* more results to fetch, we show the button.
*/
const canLoadMore = computed(() => mediaStore.canLoadMore)
const isFetching = computed(() => mediaStore.isFetching)
const reachResultEndEventSent = ref(false)
/**
Expand All @@ -56,10 +50,6 @@ const reachResultEndEventSent = ref(false)
*
*/
const onLoadMore = async () => {
if (props.isFetching) {
return
}
reachResultEndEventSent.value = false
$sendCustomEvent("LOAD_MORE_RESULTS", eventPayload.value)
Expand All @@ -76,7 +66,7 @@ const sendReachResultEnd = () => {
}
const buttonLabel = computed(() =>
props.isFetching ? t("browsePage.loading") : t("browsePage.load")
isFetching.value ? t("browsePage.loading") : t("browsePage.load")
)
const mainPageElement = ref<HTMLElement | null>(null)
onMounted(() => {
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/components/VMediaInfo/VRelatedMedia.vue
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ watch(
{ immediate: true }
)
const isFetching = computed(() => relatedMediaStore.fetchState.isFetching)
const isFetching = computed(() => relatedMediaStore.isFetching)
const showRelated = computed(
() => results.value.items.length > 0 || isFetching.value
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ defineProps<{
isFetching: boolean
searchTerm: string
creatorUrl?: string
canLoadMore: boolean
}>()
defineEmits<{ "load-more": [] }>()
Expand All @@ -40,6 +41,7 @@ defineEmits<{ "load-more": [] }>()
<template #footer>
<footer class="mb-6 mt-4 lg:mb-10">
<VLoadMore
:can-load-more="canLoadMore"
:search-type="results.type"
kind="collection"
:search-term="searchTerm"
Expand Down
18 changes: 15 additions & 3 deletions frontend/src/components/VSearchResultsGrid/VSearchResults.vue
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<script setup lang="ts">
import { useI18n } from "#imports"
import { useI18n, useI18nResultsCount } from "#imports"
import { computed } from "vue"
import type { SupportedMediaType } from "#shared/constants/media"
Expand Down Expand Up @@ -33,7 +33,19 @@ const collectionLabel = computed(() => {
const contentLinkPath = (mediaType: SupportedMediaType) =>
searchStore.getSearchPath({ type: mediaType })
const showLoading = computed(() => mediaStore.showLoading)
const { getResultCountLabels } = useI18nResultsCount(showLoading)
const labels = (
count: number,
searchTerm: string,
mediaType: SupportedMediaType
) => getResultCountLabels(count, mediaType, searchTerm)
const resultCounts = computed(() => mediaStore.resultCountsPerMediaType)
const canLoadMore = computed(() => mediaStore.canLoadMore)
</script>

<template>
Expand Down Expand Up @@ -62,16 +74,16 @@ const resultCounts = computed(() => mediaStore.resultCountsPerMediaType)
<VContentLink
v-for="[mediaType, count] in resultCounts"
:key="mediaType"
:labels="labels(count, searchTerm, mediaType)"
:media-type="mediaType"
:search-term="searchTerm"
:results-count="count"
:to="contentLinkPath(mediaType)"
/>
</div>
</template>
<template #footer>
<footer class="mb-6 mt-4 lg:mb-10">
<VLoadMore
:can-load-more="canLoadMore"
:search-type="results.type"
kind="search"
:search-term="searchTerm"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { h } from "vue"

import { useMediaStore } from "~/stores/media"

import VLoadMore from "~/components/VLoadMore.vue"

import type { StoryObj } from "@storybook/vue3"
Expand All @@ -10,16 +8,14 @@ const Template: Story = {
render: (args) => ({
components: { VLoadMore },
setup() {
const mediaStore = useMediaStore()
mediaStore._startFetching("image")
mediaStore.results.image.count = 1
return () =>
h("div", { class: "flex p-4", id: "wrapper" }, [
h(VLoadMore, {
kind: "search",
searchType: "image",
searchTerm: "cat",
isFetching: false,
canLoadMore: true,
...args,
}),
])
Expand Down
9 changes: 5 additions & 4 deletions frontend/src/composables/use-collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export const useCollection = <T extends SupportedMediaType>({
const searchStore = useSearchStore()

const collectionParams = computed(() => searchStore.collectionParams)
const isFetching = computed(() => mediaStore.fetchState.isFetching)
const isFetching = computed(() => mediaStore.isFetching)

const media = ref(mediaStore.resultItems[mediaType]) as Ref<ResultType[]>
const creatorUrl = ref<string>()
Expand All @@ -38,9 +38,8 @@ export const useCollection = <T extends SupportedMediaType>({
shouldPersistMedia: false,
}
) => {
media.value = (await mediaStore.fetchMedia({
shouldPersistMedia,
})) as ResultType[]
const results = await mediaStore.fetchMedia({ shouldPersistMedia })
media.value = results.items as ResultType[]
creatorUrl.value =
media.value.length > 0 ? media.value[0].creator_url : undefined
return media.value
Expand All @@ -53,6 +52,7 @@ export const useCollection = <T extends SupportedMediaType>({
await fetchMedia()
})

const canLoadMore = computed(() => mediaStore.canLoadMore)
const loadMore = async () => {
await fetchMedia({ shouldPersistMedia: true })
}
Expand All @@ -71,5 +71,6 @@ export const useCollection = <T extends SupportedMediaType>({
media,
fetchMedia,
loadMore,
canLoadMore,
}
}
Loading

0 comments on commit 6b87911

Please sign in to comment.