-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: Default data for local file nodes #4426
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
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| file_count_limit: 50, | ||
| } | ||
| const form_data = computed({ |
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 provided code does not contain any immediate irregularities, but there are some areas where it can be improved or optimized:
-
Prop Default Values: The
nodeModelprop is defined withany, which should ideally specify a more specific type for better TypeScript type checking. -
Form Initialization: While the current initial values of
form_type_listandfile_count_limitseem appropriate, having these hard-coded in JavaScript literals can make it harder to modify later if needed. Consider using an environment variable or configuration file to manage static data like this. -
Computed Properties: Using
computedto calculate properties that only depend on reactive data is generally good practice. However, ensure there's no unnecessary computation happening within these computed properties. -
Code Style Consistency: Ensure consistent spacing and indentation throughout the codebase, as it improves readability and maintainability.
Here's a slightly modified version with some of these considerations:
import { defineProps, computed } from 'vue';
const props = defineProps<{ nodeModel: any }>();
// Define static constants outside components to improve reusability and maintainability
export const FILE_TYPE_LIST_OPTIONS = [
'TXT',
'DOCX',
'PDF',
'HTML',
'XLS',
'XLSX',
'ZIP',
'CSV'
];
const form = {
file_type_list: FILE_TYPE_LIST_OPTIONS,
// Set reasonable limits for file size and count considering typical use cases
file_size_limit: 100 * 1024 * 1024, // e.g., 100 MB
file_count_limit: 50,
};
const formData = computed(() => ({
selectedFileTypeList: getSelectedFileTypes(), // Implement a helper function separately
}));
// Helper function to filter out unselected types dynamically based on user input
function getSelectedFileTypes() {
// Example logic: return checked/selected file types here
}Summary:
- Use
definePropsconsistently throughout your Vue.js application. - Separate dynamic configuration (like default values) into modules or external files.
- Avoid redundant computations within computed properties unless absolutely necessary.
- Maintain consistency through proper coding style practices.
| node_data: {}, | ||
| config: { | ||
| fields: [ | ||
| { |
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 major irregularities or significant issues in the provided code snippet. However, there is an unnecessary property (node_data) being defined within the config object. This could be removed as it seems to serve no purpose at this point.
Here's a revised version of the code that removes the unused node_data property:
// ...
export const dataSourceLocalNode = {
height: 728.375,
stepName: t('views.workflow.nodes.dataSourceLocalNode.label'),
input_field_list: [],
// Removed: node_data: {},
config: {
fields: [
{
// ...
}
],
// Other configuration properties...
},
};This change reduces redundancy and maintains clarity in the component's structure.
fix: Default data for local file nodes