Skip to content

fix: Form default value display error #1950

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

Merged
shaohuzhang1 merged 1 commit into from
Dec 30, 2024
+14 −2
Merged

fix: Form default value display error #1950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion ui/src/components/dynamics-form/FormItem.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ const rules = computed(() => {
? props_info.value.rules.map(to_rule)
: {
message: errMsg.value,
trigger: ['blur', 'change'],
trigger: props.formfield.input_type === 'Slider' ? 'blur' : ['blur', 'change'],
required: props.formfield.required === false ? false : true
}
})
Copy link
Contributor Author

@shaohuzhang1 shaohuzhang1 Dec 30, 2024

Choose a reason for hiding this comment

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

The code looks mostly correct. However, there are a few minor improvements you might consider:

  1. Destructuring: You can destructure props inside the functional component to make it cleaner.

    const { formfield } = props;

  2. Consistent Spacing and Indentation: Ensure consistent spacing around operators and braces for better readability.

  3. Type Check: If formfield.input_type is expected to be 'Slider', ensure that it's actually checked with .includes() since strings use a comma-separated list when split by ,.

  4. Comments: Consider adding comments if they don't already explain the purpose of each step clearly.

Here's an improved version:

import { computed, ref, reactive, watchEffect } from 'vue';

export default defineComponent({
  name: 'DynamicForm',
  setup(props) {
    // Destructure props to improve readability
    const { formfield } = props;

    // ... rest of your existing code ...
  }
});

This doesn't contain any irregularities or significant optimizations needed based on the initial review, but these changes will enhance the maintainability of your codebase.

Expand Down
10 changes: 10 additions & 0 deletions ui/src/components/dynamics-form/constructor/items/SliderConstructor.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,16 @@ const getData = () => {
'show-input-controls': false,
'show-input': formValue.value.showInput
},
props_info: {
rules: [
{
message: formValue.value.label + '不能为空',
trigger: 'blur',
required: formValue.value.required
}
]
},
show_default_value: true,
default_value: formValue.value.default_value
}
}
Copy link
Contributor Author

@shaohuzhang1 shaohuzhang1 Dec 30, 2024

Choose a reason for hiding this comment

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

The provided code snippet seems to be related to configuring a reactive component that appears to manage form fields. There are a few suggestions and improvements that can be made:

  1. Simplify Conditional Rendering: Combine conditions within the same object properties, if applicable:

    'show-default-value': !!formValue.value.hasOwnProperty('default_value')

  2. Consistent Use of required Property: Ensure consistency with use of the required property across different components or data structures.

  3. Documentation and Comments: Add necessary comments to explain the purpose of each configuration option if it is not immediately clear.

Here is an optimized version based on these suggestions:

const getData = () => {
  return {
    // ... other configurations...
    form_control_options: {
      hide_input_controls: !formValue.value.showInputControls,
      show_input: Boolean(formValue.value.showInput),
+     required_rules: formValue.value.required ? [{
+         message: formValue.value.label || "Field is required",
+         trigger: "blur"
+       }] : [],
+     enable_default_value_showing: typeof formValue.default_value !== "undefined" && formValue.default_value !== null,
     initial_form_values: formValue.toObject() // Assuming formValue.toObject returns default values from the input form
  };
};

This revision simplifies conditional rendering for show-default-value, ensures consistent usage of required within rules configuration using array notation where appropriate, and adds type checking before setting defaults. However, note that assumptions about how toObject() works need to align with actual implementation details in your project context.

Expand Down
1 change: 1 addition & 0 deletions ui/src/components/dynamics-form/constructor/items/SwitchInputConstructor.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const formValue = computed({
const getData = () => {
return {
input_type: 'SwitchInput',
show_default_value: true,
attrs: {},
default_value: formValue.value.default_value
}
Expand Down
3 changes: 2 additions & 1 deletion ui/src/components/dynamics-form/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,12 +181,13 @@ const render = (
if (form_data[item.field] !== undefined) {
return { [item.field]: form_data[item.field] }
}
if (item.show_default_value) {
if (item.show_default_value === true || item.show_default_value === undefined) {
return { [item.field]: item.default_value }
}
return {}
})
.reduce((x, y) => ({ ...x, ...y }), {})

formValue.value = _.cloneDeep(value)
}
}
Copy link
Contributor Author

@shaohuzhang1 shaohuzhang1 Dec 30, 2024

Choose a reason for hiding this comment

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

The code looks generally clean and should work as intended. However, there are a few minor improvements you can make:

  1. Simplify Conditional Checks:
    You can use a single condition to avoid duplication:

    if (item.show_default_value || item.default_value !== null && item.default_value !== undefined) {
  return { [item.field]: item.default_value };
}

  2. Use Object.assign Instead of Spread Operator:
    While spread operators like {...x, ...y} perform well for shallow copies, they may require additional checks or handling in some cases. It might be more reliable to use Object.assign, which also handles non-array objects gracefully.

  3. Avoid Cloning Values:
    If the intention is not to modify the original data, cloning values is unnecessary in this context since you're using immutable operations throughout. You can simplify it to:

    formValue.value = value;

Here's the updated code with these suggestions:

@@ -181,12 +181,15 @@ const render = (
       .map(item => {
         if (form_data[item.field] !== undefined) {
           return { [item.field]: form_data[item.field] };
         }
         // Simplified conditional check
         if (item.show_default_value || item.default_value !== null && item.default_value !== undefined) {
           return { [item.field]: item.default_value };
         }
         return {};
       })
       .reduce((x, y) => Object.assign({}, x, y), {});

     // Remove cloneDeep as we don't need to mutate the value
     formValue.value = value;
   }
 }

These changes enhance readability and maintainability while ensuring correctness.

Expand Down
Loading