Skip to content

Commit

Permalink
Merge pull request #16738 from davelopez/explore_collection_item_scro…
Browse files Browse the repository at this point in the history 
…ll_improvements

Improve collection items scrolling
  • Loading branch information
@bgruening
bgruening authored Oct 14, 2023
2 parents 80cc5dd + 3b19e65 commit cde7658
Show file tree
Hide file tree
Showing 6 changed files with 168 additions and 73 deletions.
21 changes: 14 additions & 7 deletions client/src/components/History/Content/ContentItem.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
</b-badge>
</span>
<ContentOptions
v-else
v-if="!isPlaceholder && !item.purged"
:writable="writable"
:is-dataset="isDataset"
:is-deleted="item.deleted"
Expand Down Expand Up @@ -102,7 +102,7 @@

<script>
import { library } from "@fortawesome/fontawesome-svg-core";
import { faArrowCircleDown, faArrowCircleUp, faCheckCircle } from "@fortawesome/free-solid-svg-icons";
import { faArrowCircleDown, faArrowCircleUp, faCheckCircle, faSpinner } from "@fortawesome/free-solid-svg-icons";
import { FontAwesomeIcon } from "@fortawesome/vue-fontawesome";
import { updateContentFields } from "components/History/model/queries";
import StatelessTags from "components/TagsMultiselect/StatelessTags";
Expand All @@ -116,7 +116,7 @@ import ContentOptions from "./ContentOptions";
import DatasetDetails from "./Dataset/DatasetDetails";
import { HIERARCHICAL_COLLECTION_JOB_STATES, STATES } from "./model/states";
library.add(faArrowCircleUp, faArrowCircleDown, faCheckCircle);
library.add(faArrowCircleUp, faArrowCircleDown, faCheckCircle, faSpinner);
export default {
components: {
CollectionDescription,
Expand All @@ -126,18 +126,19 @@ export default {
FontAwesomeIcon,
},
props: {
id: { type: Number, required: true },
item: { type: Object, required: true },
name: { type: String, required: true },
expandDataset: { type: Boolean, default: false },
writable: { type: Boolean, default: true },
expandDataset: { type: Boolean, required: true },
addHighlightBtn: { type: Boolean, default: false },
highlight: { type: String, default: null },
id: { type: Number, required: true },
isDataset: { type: Boolean, default: true },
isHistoryItem: { type: Boolean, default: false },
item: { type: Object, required: true },
name: { type: String, required: true },
selected: { type: Boolean, default: false },
selectable: { type: Boolean, default: false },
filterable: { type: Boolean, default: false },
isPlaceholder: { type: Boolean, default: false },
},
computed: {
jobState() {
Expand Down Expand Up @@ -166,6 +167,9 @@ export default {
return this.contentState && this.contentState.icon;
},
state() {
if (this.isPlaceholder) {
return "placeholder";
}
if (this.item.job_state_summary) {
for (const state of HIERARCHICAL_COLLECTION_JOB_STATES) {
if (this.item.job_state_summary[state] > 0) {
Expand Down Expand Up @@ -219,6 +223,9 @@ export default {
}
},
onClick() {
if (this.isPlaceholder) {
return;
}
if (this.isDataset) {
this.$emit("update:expand-dataset", !this.expandDataset);
} else {
Expand Down
11 changes: 9 additions & 2 deletions client/src/components/History/Content/model/states.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ import { components } from "@/schema";

type DatasetState = components["schemas"]["DatasetState"];
// The 'failed' state is for the collection job state summary, not a dataset state.
type State = DatasetState | "failed";
type State = DatasetState | "failed" | "placeholder";

interface StateRepresentation {
status: "success" | "warning" | "info" | "danger";
status: "success" | "warning" | "info" | "danger" | "secondary";
text?: string;
icon?: string;
spin?: boolean;
Expand Down Expand Up @@ -97,6 +97,13 @@ export const STATES: StateMap = {
status: "danger",
icon: "exclamation-triangle",
},
/** the dataset is not yet loaded in the UI. This state is only visual and transitional, it does not exist in the database. */
placeholder: {
status: "secondary",
text: "This dataset is being fetched.",
icon: "spinner",
spin: true,
},
} as const satisfies StateMap;

/** We want to display a single state for a dataset collection whose elements may have mixed states.
Expand Down
13 changes: 12 additions & 1 deletion client/src/components/History/CurrentCollection/CollectionPanel.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,20 @@ watch(
</section>
<section class="position-relative flex-grow-1 scroller">
<div>
<ListingLayout :items="collectionElements" :loading="loading" @scroll="onScroll">
<ListingLayout
data-key="element_index"
:items="collectionElements"
:loading="loading"
@scroll="onScroll">
<template v-slot:item="{ item }">
<ContentItem
v-if="item.id === undefined"
:id="item.element_index + 1"
:item="item"
:is-placeholder="true"
name="Loading..." />
<ContentItem
v-else
:id="item.element_index + 1"
:item="item.object"
:name="item.element_identifier"
Expand Down
3 changes: 2 additions & 1 deletion client/src/components/History/Layout/ListingLayout.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
ref="listing"
class="listing"
role="list"
data-key="id"
:data-key="dataKey"
:offset="offset"
:data-sources="items"
:data-component="{}"
Expand Down Expand Up @@ -32,6 +32,7 @@ export default {
VirtualList,
},
props: {
dataKey: { type: String, default: "id" },
offset: { type: Number, default: 0 },
loading: { type: Boolean, default: false },
items: { type: Array, default: null },
Expand Down
89 changes: 56 additions & 33 deletions client/src/stores/collectionElementsStore.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,11 @@ import flushPromises from "flush-promises";
import { createPinia, setActivePinia } from "pinia";

import { mockFetcher } from "@/schema/__mocks__";
import { useCollectionElementsStore } from "@/stores/collectionElementsStore";
import { DCEEntry, useCollectionElementsStore } from "@/stores/collectionElementsStore";
import { DCESummary, HDCASummary } from "@/stores/services";

jest.mock("@/schema");

const collection1: HDCASummary = mockCollection("1");
const collection2: HDCASummary = mockCollection("2");
const collections: HDCASummary[] = [collection1, collection2];

describe("useCollectionElementsStore", () => {
beforeEach(() => {
setActivePinia(createPinia());
Expand All @@ -21,6 +17,9 @@ describe("useCollectionElementsStore", () => {
});

it("should save collections", async () => {
const collection1: HDCASummary = mockCollection("1");
const collection2: HDCASummary = mockCollection("2");
const collections: HDCASummary[] = [collection1, collection2];
const store = useCollectionElementsStore();
expect(store.storedCollections).toEqual({});

Expand All @@ -33,63 +32,79 @@ describe("useCollectionElementsStore", () => {
});

it("should fetch collection elements if they are not yet in the store", async () => {
const totalElements = 10;
const collection: HDCASummary = mockCollection("1", totalElements);
const store = useCollectionElementsStore();
expect(store.storedCollectionElements).toEqual({});
expect(store.isLoadingCollectionElements(collection1)).toEqual(false);
expect(store.isLoadingCollectionElements(collection)).toEqual(false);

const offset = 0;
const limit = 5;
// Getting collection elements should trigger a fetch
store.getCollectionElements(collection1, offset, limit);
expect(store.isLoadingCollectionElements(collection1)).toEqual(true);
// Getting collection elements should trigger a fetch and change the loading state
store.getCollectionElements(collection, offset, limit);
expect(store.isLoadingCollectionElements(collection)).toEqual(true);
await flushPromises();
expect(store.isLoadingCollectionElements(collection1)).toEqual(false);
expect(store.isLoadingCollectionElements(collection)).toEqual(false);
expect(fetchCollectionElements).toHaveBeenCalled();

const collection1Key = store.getCollectionKey(collection1);
const elements = store.storedCollectionElements[collection1Key];
const collectionKey = store.getCollectionKey(collection);
const elements = store.storedCollectionElements[collectionKey];
expect(elements).toBeDefined();
expect(elements).toHaveLength(limit);
// The total number of elements (including placeholders) is 10, but only the first 5 are fetched (real elements)
expect(elements).toHaveLength(totalElements);
const nonPlaceholderElements = getRealElements(elements);
expect(nonPlaceholderElements).toHaveLength(limit);
});

it("should not fetch collection elements if they are already in the store", async () => {
const totalElements = 10;
const collection: HDCASummary = mockCollection("1", totalElements);
const store = useCollectionElementsStore();
// Prefill the store with the first 5 elements
const storedCount = 5;
const expectedStoredElements = Array.from({ length: storedCount }, (_, i) => mockElement(collection1.id, i));
const collection1Key = store.getCollectionKey(collection1);
store.storedCollectionElements[collection1Key] = expectedStoredElements;
expect(store.storedCollectionElements[collection1Key]).toHaveLength(storedCount);
const expectedStoredElements = Array.from({ length: storedCount }, (_, i) => mockElement(collection.id, i));
const collectionKey = store.getCollectionKey(collection);
store.storedCollectionElements[collectionKey] = expectedStoredElements;
expect(store.storedCollectionElements[collectionKey]).toHaveLength(storedCount);

const offset = 0;
const limit = 5;
// Getting collection elements should not trigger a fetch in this case
store.getCollectionElements(collection1, offset, limit);
expect(store.isLoadingCollectionElements(collection1)).toEqual(false);
const limit = storedCount;
// Getting the same collection elements range should not trigger a fetch
store.getCollectionElements(collection, offset, limit);
expect(store.isLoadingCollectionElements(collection)).toEqual(false);
expect(fetchCollectionElements).not.toHaveBeenCalled();
});

it("should fetch only missing elements if the requested range is not already stored", async () => {
const totalElements = 10;
const collection: HDCASummary = mockCollection("1", totalElements);
const store = useCollectionElementsStore();
const storedCount = 3;
const expectedStoredElements = Array.from({ length: storedCount }, (_, i) => mockElement(collection1.id, i));
const collection1Key = store.getCollectionKey(collection1);
store.storedCollectionElements[collection1Key] = expectedStoredElements;
expect(store.storedCollectionElements[collection1Key]).toHaveLength(storedCount);

const initialElements = 3;
store.getCollectionElements(collection, 0, initialElements);
await flushPromises();
expect(fetchCollectionElements).toHaveBeenCalled();
const collectionKey = store.getCollectionKey(collection);
let elements = store.storedCollectionElements[collectionKey];
// The first call will initialize the 10 placeholders and fetch the first 3 elements out of 10
expect(elements).toHaveLength(totalElements);
expect(getRealElements(elements)).toHaveLength(initialElements);

const offset = 2;
const limit = 5;
// Getting collection elements should trigger a fetch in this case
store.getCollectionElements(collection1, offset, limit);
expect(store.isLoadingCollectionElements(collection1)).toEqual(true);
store.getCollectionElements(collection, offset, limit);
expect(store.isLoadingCollectionElements(collection)).toEqual(true);
await flushPromises();
expect(store.isLoadingCollectionElements(collection1)).toEqual(false);
expect(store.isLoadingCollectionElements(collection)).toEqual(false);
expect(fetchCollectionElements).toHaveBeenCalled();

const elements = store.storedCollectionElements[collection1Key];
elements = store.storedCollectionElements[collectionKey];
expect(elements).toBeDefined();
expect(elements).toHaveLength(10);
// The offset was overlapping with the stored elements, so it was increased by the number of stored elements
// so it fetches the next "limit" number of elements
expect(elements).toHaveLength(storedCount + limit);
// and it fetches the next "limit" number of elements
expect(getRealElements(elements)).toHaveLength(initialElements + limit);
});
});

Expand All @@ -100,7 +115,7 @@ function mockCollection(id: string, numElements = 10): HDCASummary {
collection_type: "list",
populated_state: "ok",
populated_state_message: "",
collection_id: id,
collection_id: `DC_ID_${id}`,
name: `collection ${id}`,
deleted: false,
contents_url: "",
Expand Down Expand Up @@ -138,6 +153,7 @@ function mockElement(collectionId: string, i: number): DCESummary {

interface ApiRequest {
hdca_id: string;
parent_id: string;
offset: number;
limit: number;
}
Expand All @@ -155,3 +171,10 @@ function fakeCollectionElementsApiResponse(params: ApiRequest) {
data: elements,
};
}

/**
* Filter out the placeholder elements from the given array.
*/
function getRealElements(elements?: DCEEntry[]): DCESummary[] | undefined {
return elements?.filter((element) => "id" in element === true) as DCESummary[];
}
Loading

0 comments on commit cde7658

Please sign in to comment.