Skip to content

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

Merged
merged 1 commit into from
May 19, 2025
Merged

fix: breadcrumb issue #3111

merged 1 commit into from
May 19, 2025

Conversation

wangdan-fit2cloud
Copy link
Contributor

What this PR does / why we need it?

Summary of your change

Please indicate you've done the following:

  • Made sure tests are passing and test coverage is added if needed.
  • Made sure commit message follow the rule of Conventional Commits specification.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed.

@wangdan-fit2cloud wangdan-fit2cloud merged commit a30316d into main May 19, 2025
4 checks passed
@wangdan-fit2cloud wangdan-fit2cloud deleted the pr@main/breadcrumb-issue branch May 19, 2025 10:18
getDataset()
} else if (isApplication.value) {
getApplication()
}
}
onMounted(() => {
if (!breadcrumbData.value) {
Copy link
Contributor

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:

  1. Ensure that all conditional blocks end with an indentation change.
  2. 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 {
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants