fix(metadata-sidebar): fix loading states #3684
Conversation
src/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts
Outdated
Show resolved
Hide resolved
wpiesiak
force-pushed
the
metadata-fix-loading
branch
from
4f8d256
to
aa1e43b
Compare
| @@ -93,7 +93,7 @@ function MetadataSidebarRedesign({ | |||
|
|
|||
| React.useEffect(() => { | |||
| setSelectedTemplates(templateInstances); | |||
| }, [templateInstances]); | |||
| }, [templateInstances, templateInstances.length]); | |||
There was a problem hiding this comment.
Is templateInstances.length needed? Shouldn't having only templateInstances as a dependency observe changes in the size of the array as well?
There was a problem hiding this comment.
templateInstance is an array from api cache, therefore changes in the array would not trigger this useEffect
| setStatus(STATUS.LOADING); | ||
| await api | ||
| .getMetadataAPI(false) | ||
| .deleteMetadata(file, metadataInstance, () => setStatus(STATUS.SUCCESS), onApiError, true); |
There was a problem hiding this comment.
❓ 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?
There was a problem hiding this comment.
templateInstances are linked to an array in api cache and are updated in the Metadata.js
| successCallback, | ||
| (error: ElementsXhrError, code: string) => | ||
| onApiError(error, code, messages.sidebarMetadataEditingErrorContent), | ||
| ); | ||
| }, | ||
| [api, file, onApiError], | ||
| ); | ||
|
|
||
| const handleUpdateMetadataInstance = React.useCallback( |
There was a problem hiding this comment.
❓ I imagine updates and creates should also have a loading states but don't see any setStatus usage (like delete). Should this be added?
There was a problem hiding this comment.
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.
| ) | ||
| : handleCreateMetadataInstance(values.metadata as MetadataTemplateInstance, () => setEditingTemplate(null)); | ||
| if (isExistingMetadataInstance()) { | ||
| await handleUpdateMetadataInstance(values.metadata as MetadataTemplateInstance, operations, () => |
There was a problem hiding this comment.
Can we use convertTemplateToTemplateInstance instead of casting here as it's already merged ?
There was a problem hiding this comment.
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, () => |
There was a problem hiding this comment.
Same here, let's use convertTemplateToTemplateInstance
Fix loading states for create, update and delete metadata instance