-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: breadcrumb issue #3111
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.
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
fix: breadcrumb issue #3111
Conversation
getDataset() | ||
} else if (isApplication.value) { | ||
getApplication() | ||
} | ||
} | ||
onMounted(() => { | ||
if (!breadcrumbData.value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refresh()
function has a logic issue. It checks whether isDataset.value
is true, then calls getDataset()
. If not, it checks isApplication.value
, which should be checked only if isDataset.value
was false to avoid calling both functions.
For better readability and maintainability:
- Ensure that all conditional blocks end with an indentation change.
- Add comments explaining the conditional logic.
Here's a suggested revision:
function refresh() {
common.saveBreadcrumb(null);
if (isDataset.value) {
// Update dataset logic here if needed
getDataset();
} else if (isApplication.value && !isDataset.value) {
// Update application logic here if needed
getApplication();
} else {
console.error("Unexpected value of isDataset and isApplication");
}
}
Additionally, consider adding error handling when making network requests in your getApplication()
or getDataset()
calls. This will help manage edge cases more gracefully during development and production environments.
@@ -242,6 +242,7 @@ const submitHandle = async (formEl: FormInstance | undefined) => { | |||
} | |||
applicationApi.postApplication(applicationForm.value, loading).then((res) => { | |||
MsgSuccess(t('common.createSuccess')) | |||
emit('refresh') | |||
if (isWorkFlow(applicationForm.value.type)) { | |||
router.push({ path: `/application/${res.data.id}/workflow` }) | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no apparent errors in the provided code snippet regarding logic or syntax. However, there is a suggestion for an additional optimization:
Optimization Suggestion:
Before calling emit('refresh')
, you might consider checking if there's an instance of a component that can handle this refresh event to avoid unnecessary emissions.
Here’s how the updated function could look with this consideration added:
const submitHandler = async (formEl: FormInstance | undefined) => {
if(formEl?.validateForm()) { // Ensure form validation passes before proceeding
applicationApi.postApplication(applicationForm.value, loading).then((res) => {
MsgSuccess(t('common.createSuccess'));
if(isWorkFlow(applicationForm.value.type)) {
router.push({ path: `/application/${res.data.id}/workflow` });
} else {
// Check if there's a specific component that needs refreshing
const componentsToRefresh = [/* List of component IDs here */];
const foundComponent = document.getElementById(componentsToRefresh.find(componentId =>
!!document.querySelector(`#${componentId}`)
)) as HTMLElement;
if(foundComponent || window.location.reload) {
emit('refresh');
if(window.location.reload && !foundComponent)
window.location.reload();
}
}
}).catch(errorHandling); // Handle any other error scenario(s)
}
};
This enhancement ensures that only when necessary updates occur, leading to potentially improved performance in scenarios where multiple parts of the UI need to be refreshed due to changes related to workflow creation.
What this PR does / why we need it?
Summary of your change
Please indicate you've done the following: