Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(metadata-sidebar): fix loading states #3684

Merged
merged 4 commits into from
Oct 4, 2024
Merged

fix(metadata-sidebar): fix loading states #3684

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
961a372
fix(metadata-sidebar): fix loading states
wpiesiak Sep 26, 2024
aa1e43b
fix(metadata-sidebar): fix loading states
wpiesiak Sep 30, 2024
9344ff8
Merge branch 'master' into metadata-fix-loading
mergify[bot] Oct 3, 2024
8877652
Merge branch 'master' into metadata-fix-loading
wpiesiak Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions src/elements/content-sidebar/MetadataSidebarRedesign.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ function MetadataSidebarRedesign({

React.useEffect(() => {
setSelectedTemplates(templateInstances);
}, [templateInstances]);
}, [templateInstances, templateInstances.length]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is templateInstances.length needed? Shouldn't having only templateInstances as a dependency observe changes in the size of the array as well?

Copy link
Contributor Author

@wpiesiak wpiesiak Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

templateInstance is an array from api cache, therefore changes in the array would not trigger this useEffect


const handleUnsavedChanges = () => {
setIsUnsavedChangesModalOpen(true);
Expand All @@ -105,8 +105,8 @@ function MetadataSidebarRedesign({
setIsDeleteButtonDisabled(true);
};

const handleDeleteInstance = (metadataInstance: MetadataTemplateInstance) => {
handleDeleteMetadataInstance(metadataInstance);
const handleDeleteInstance = async (metadataInstance: MetadataTemplateInstance) => {
await handleDeleteMetadataInstance(metadataInstance);
setEditingTemplate(null);
};

Expand All @@ -117,11 +117,15 @@ function MetadataSidebarRedesign({
};

const handleSubmit = async (values: FormValues, operations: JSONPatchOperations) => {
isExistingMetadataInstance()
? handleUpdateMetadataInstance(values.metadata as MetadataTemplateInstance, operations, () =>
setEditingTemplate(null),
)
: handleCreateMetadataInstance(values.metadata as MetadataTemplateInstance, () => setEditingTemplate(null));
if (isExistingMetadataInstance()) {
await handleUpdateMetadataInstance(values.metadata as MetadataTemplateInstance, operations, () =>
Copy link
Contributor

@JakubKida JakubKida Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use convertTemplateToTemplateInstance instead of casting here as it's already merged ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

values.metadata has all required fields for MetadataTemplateInstance, we just need to formally convert the type. There is no reason here to use convertTemplateToTemplateInstance 🤔

setEditingTemplate(null),
);
} else {
await handleCreateMetadataInstance(values.metadata as MetadataTemplateInstance, () =>
Copy link
Contributor

@JakubKida JakubKida Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, let's use convertTemplateToTemplateInstance

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above ⬆️

setEditingTemplate(null),
);
}
};

const metadataDropdown = status === STATUS.SUCCESS && templates && (
Expand Down
2 changes: 1 addition & 1 deletion src/elements/content-sidebar/__tests__/useSidebarMetadataFetcher.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ describe('useSidebarMetadataFetcher', () => {
await waitFor(() => result.current.handleDeleteMetadataInstance(mockTemplateInstances[0]));

expect(result.current.templates).toEqual(mockTemplates);
expect(result.current.templateInstances).toEqual([]);
expect(result.current.status).toEqual(STATUS.SUCCESS);
expect(result.current.errorMessage).toBeNull();
});

Expand Down
81 changes: 38 additions & 43 deletions src/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,16 @@ export enum STATUS {
interface DataFetcher {
errorMessage: MessageDescriptor | null;
file: BoxItem | null;
handleCreateMetadataInstance: (templateInstance: MetadataTemplateInstance, successCallback: () => void) => void;
handleDeleteMetadataInstance: (metadataInstance: MetadataTemplateInstance) => void;
handleCreateMetadataInstance: (
templateInstance: MetadataTemplateInstance,
successCallback: () => void,
) => Promise<void>;
handleDeleteMetadataInstance: (metadataInstance: MetadataTemplateInstance) => Promise<void>;
handleUpdateMetadataInstance: (
metadataTemplateInstance: MetadataTemplateInstance,
JSONPatch: Array<Object>,
successCallback: () => void,
) => void;
) => Promise<void>;
status: STATUS;
templateInstances: Array<MetadataTemplateInstance>;
templates: Array<MetadataTemplate>;
Expand Down Expand Up @@ -122,59 +125,51 @@ function useSidebarMetadataFetcher(
[onApiError],
);

const deleteMetadataInstanceSuccessCallback = React.useCallback(
(metadataInstance: MetadataTemplateInstance) => {
const updatedInstances = templateInstances.filter(
templateInstance =>
templateInstance.scope !== metadataInstance.scope &&
templateInstance.templateKey !== metadataInstance.templateKey,
);
setTemplateInstances(updatedInstances);
},
[templateInstances],
);

const handleDeleteMetadataInstance = React.useCallback(
(metadataInstance: MetadataTemplateInstance) => {
async (metadataInstance: MetadataTemplateInstance): Promise<void> => {
if (!file || !metadataInstance) {
return;
}

api.getMetadataAPI(false).deleteMetadata(
file,
metadataInstance,
deleteMetadataInstanceSuccessCallback,
onApiError,
true,
);
setStatus(STATUS.LOADING);
await api
.getMetadataAPI(false)
.deleteMetadata(file, metadataInstance, () => setStatus(STATUS.SUCCESS), onApiError, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ seems like this is now just setting that status to success instead of updating the template instances. We no longer need to update template instances?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

templateInstances are linked to an array in api cache and are updated in the Metadata.js

},
[api, onApiError, file, deleteMetadataInstanceSuccessCallback],
[api, onApiError, file],
);

const handleCreateMetadataInstance = React.useCallback(
(templateInstance: MetadataTemplateInstance, successCallback): void => {
api.getMetadataAPI(false).createMetadataRedesign(
file,
templateInstance,
successCallback,
(error: ElementsXhrError, code: string) =>
onApiError(error, code, messages.sidebarMetadataEditingErrorContent),
);
async (templateInstance: MetadataTemplateInstance, successCallback: () => void): Promise<void> => {
await api
jankowiakdawid marked this conversation as resolved.
Show resolved Hide resolved
.getMetadataAPI(false)
.createMetadataRedesign(
file,
templateInstance,
successCallback,
(error: ElementsXhrError, code: string) =>
onApiError(error, code, messages.sidebarMetadataEditingErrorContent),
);
},
[api, file, onApiError],
);

const handleUpdateMetadataInstance = React.useCallback(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ I imagine updates and creates should also have a loading states but don't see any setStatus usage (like delete). Should this be added?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For creating and updating we pass these async functions to MetadataInstanceForm in metadata-editor. From there, Formik will keep loading state until it receives either a resolve or reject status from the API.

(metadataInstance: MetadataTemplateInstance, JSONPatch: JSONPatchOperations, successCallback: () => void) => {
api.getMetadataAPI(false).updateMetadataRedesign(
file,
metadataInstance,
JSONPatch,
successCallback,
(error: ElementsXhrError, code: string) => {
onApiError(error, code, messages.sidebarMetadataEditingErrorContent);
},
);
async (
metadataInstance: MetadataTemplateInstance,
JSONPatch: JSONPatchOperations,
successCallback: () => void,
) => {
await api
jankowiakdawid marked this conversation as resolved.
Show resolved Hide resolved
.getMetadataAPI(false)
.updateMetadataRedesign(
file,
metadataInstance,
JSONPatch,
successCallback,
(error: ElementsXhrError, code: string) => {
onApiError(error, code, messages.sidebarMetadataEditingErrorContent);
},
);
},
[api, file, onApiError],
);
Expand Down
Loading