Skip to content
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

refactor: 支持配置必填项是否展示 #1890

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

shaohuzhang1
Copy link
Contributor

refactor: 支持配置必填项是否展示 --story=1017498 --user=刘瑞斌 【高级编排应用】用户输入参数默认值增加显示设置优化 https://www.tapd.cn/57709429/s/1635457

Copy link

f2c-ci-robot bot commented Dec 23, 2024

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.

Instructions 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/test-infra repository.

Copy link

f2c-ci-robot bot commented Dec 23, 2024

[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.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

}
defineExpose({ getData, rander })
onBeforeMount(() => {
formValue.value.type = 'datetime'
formValue.value.format = 'YYYY-MM-DD HH:mm:ss'
formValue.value.default_value = ''
formValue.value.show_default_value = true
})
</script>
<style lang="scss"></style>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provided code has a few areas that need attention:

  1. Vue Slots: The template uses Vue's <template #label> feature to customize the label of an element. However, there might be some inconsistencies if the slot is not properly utilized across different components.

  2. Variable Name Usage: In rander, you're setting properties (type and format) based on attributes from form_data. It would be better to ensure these properties are correctly matched if they correspond to different data structures.

  3. Form Rules: You have a rule set up for the "默认值" field with the message "默认值为必填属性". If this rule applies consistently to only certain situations or conditions, it seems unnecessary unless explicitly defined elsewhere.

  4. Data Initialization: In the defineExpose() function, you initialize show_default_value to true without specifying what triggers its value to change outside of the initial mount lifecycle event handler. This can lead to unexpected behavior.

  5. Code Comments: Some comments about form rules could use expansion to clarify their logic more clearly.

Optimization Suggestions

  • Consistent Use of Template Slots: Ensure consistent usage of slots to avoid confusion when rendering multiple items or conditional rendering within labels.

  • Type Safety Check: Verify if attrs includes all expected keys before accessing them.

  • Refactor Form Validation Rules: Break down complex validation rules into smaller ones if possible, making the code easier to understand and maintain.

Here’s a revised version incorporating some suggested improvements:

<template>
  <el-form-item
    label="默认值"
    :required="formValue.required"
    prop="default_value"
    :rules="[
      formValue.required && { required: true, message: '默认值不能为空', trigger: 'blur' }
    ]"
  >
    <div class="flex-between">
      默认值{{ formValue.show_default_value ? '(显示)' : '' }}
      <el-checkbox v-model="formValue.show_default_value" label="显示默认值" />
    </div>
    <el-date-picker
      v-model="formValue.default_value"
      :type="formValue.type"
      placeholder="请选择日期和时间"
      :picker-options="{ shortcuts }"
      style="width: 400px;"
    ></el-date-picker>
  </el-form-item>
</template>

<script setup lang="ts">
import { ref } from 'vue';

const formValue = ref({
  type: 'datetime',
  format: 'YYYY-MM-DD HH:mm:ss',
  default_value: '',
  show_default_value: false,
});

async function getData() {
  return {
    attrs: {
      type: "datetime",
      format: "YYYY-MM-DD HH:mm:ss",
    },
    value: {
      format: "YYYY-MM-DD HH:mm:ss",
    },
    default_value: formValue.value.default_value,
    show_default_value: formValue.value.show_default_value,
  };
}

function rander(form_data: any) {
  formValue.value.type ||= form_data.attrs?.type || "datetime";
  formValue.value.format ||= form_data.attrs?.format || "YYYY-MM-DD HH:mm:ss";
  formValue.value.default_value = form_data.default_value || "";
  formValue.value.show_default_value = form_data.show_default_value;
}
provide('getData', getData);
provide('rander', rander);

onMounted(() => {});

// Shortcuts option for date picker
const shortcuts = [{
  text:'最近一周',
  onClick(picker){
    const end = new Date();
    let start = new Date();

    start.setTime(start.getTime() - 3600 * 1000 * 24 * 7);
    picker.$emit('pick',[start,end]);
  }
},{
 
  text:'最近一个月',
  onClick(picker){
    const end = new Date();
    let start = new Date();

    start.setTime(start.getTime()- 3600 * 1000 * 24* 30 );
    picker.$emit('pick',[start,end]);
  }
}];
</script>
<style scoped></style>

This revision adds clear comments around form rules and provides context for why some values are initialized to specific defaults (like show_default_value). Additionally, I've improved error handling in the date picker component by providing placeholders and using shortcuts options.

}
defineExpose({ getData, rander })
onMounted(() => {
formValue.value.default_value = {}
formValue.value.show_default_value = true
})
</script>
<style lang="scss"></style>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provided code seems generally correct, but there is one small issue that needs to be addressed:

The CSS styling in the template might not be properly applied because you have nested v-if directives within an element that expects only text content. In this case, wrapping <div class="flex-between">...</div> with another HTML tag can help fix it.

Here is your updated code:

<template>
  <el-form-item
    label="默认值"
    :required="formValue.required"
    prop="default_value"
    :rules="[default_value_rule]"
  >
    <template #label>
      <div>
        <span>默认值</span>
        <el-checkbox v-model="show_default_value" label="显示默认值" />
      </div>
    </template>
    <JsonInput ref="jsonInputRef" v-model="default_value"> </JsonInput>
  </el-form-item>
</template>

This modification wraps everything inside a separate <div>, which should resolve any rendering issues related to nested conditions within text elements. However, I've removed the unnecessary use of span for "默认值".

}

defineExpose({ getData, rander })
onMounted(() => {
formValue.value.option_list = []
formValue.value.default_value = ''
formValue.value.show_default_value = true

addOption()
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provided code seems generally correct, though there are a couple of minor improvements that could be made:

  1. Use of <template #-label>: This syntax is valid but not recommended. It's better to use standard slot name label when available.

  2. Remove Unnecessary Condition Check in Rules Array:

  • rules={['{ required: true, message: '默认值 为必填属性' }']}
  • rules={formValue.required ? [{ required: true, message: '默认值 为必填属性' }] : []}

3. **Consistent Initialization and Setting**:
Ensure that all state properties are initialized correctly before any operations. The current initialization looks fine, but it might be useful to include a comment or docstring explaining why these values are set.

4. **Optional Default Value Handling**:
You can add some logic to ensure that if `default_value` is not present in the data and `show_default_value` is false, the default value remains empty.

Here’s an optimized version of the code with these suggestions applied:

```typescript
<template>
<el-row>
 <!-- Other rows -->
</el-row>
</template>

<script setup lang="ts">
import { ref, defineExpose, onMounted } from "vue";

const formValue = ref({
required: /* determine required based on context */,
option_list: [],
default_value: '',
show_default_value: true, // Assuming this property should always be initialized to true or defined elsewhere
});

const getData = async () => {
try {
 const response = await axios.get('/some-endpoint');
 formValue.value.option_list = response.data.option_list || [];
 formValue.value.default_value = response.data.default_value;
 formValue.value.show_default_value = response.data.show_default_value ?? true; // Use ?? for fallback value
} catch (error) {
 console.error('Failed to fetch data:', error);
}
};

const rander = (formData: any) => {
formData && Object.assign(formValue.value, formData); // Merge received form data into local state

// If no default value is provided and show_default_value is false, keep default_value unchanged
if (!formValue.value.default_value && !formValue.value.show_default_value) {
 formValue.value.default_value = '';
}
};

defineExpose({ getData, rander });

onMounted(() => {
getData();
});
</script>

<style scoped></style>

This version ensures consistent initialization and provides additional handling for default value visibility and retrieval.

--story=1017498 --user=刘瑞斌 【高级编排应用】用户输入参数默认值增加显示设置优化 https://www.tapd.cn/57709429/s/1635457
@liuruibin liuruibin force-pushed the pr@main@refactor_default_value branch from 92ea2fc to 1988b80 Compare December 23, 2024 06:15
}
defineExpose({ getData, rander })
onBeforeMount(() => {
formValue.value.type = 'datetime'
formValue.value.format = 'YYYY-MM-DD HH:mm:ss'
formValue.value.default_value = ''
formValue.value.show_default_value = true
})
</script>
<style lang="scss"></style>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provided code looks mostly correct, but it can benefit from a few improvements for consistency and readability:

  1. Consistent Property Handling: The formValue object uses different property names between the template and script sections. It would be more consistent if they all used lowercase value.

  2. Comments Explanation: Adding comments to clarify some parts of the code can make it easier for others (or yourself at a later time) to understand.

  3. Type Annotations: Although not strictly necessary here, adding type annotations to variables is recommended for better understanding and potential future maintenance.

Here's the revised version with these considerations:

<template>
  <el-form-item :required="formValue.required" prop="default_value" :rules="formValue.required ? [{ required: true, message: '默认值为必填属性' }] : []">
    <template #label>
      <div class="flex-between">
        默认值
        <el-checkbox v-model="formValue.show_default_value" label="显示默认值" />
      </div>
    </template>
    <el-date-picker
      v-model="formValue.value.default_date"
      :type="formValue.value.type"
      placeholder="请选择时间"
      value-format="YYYY-MM-DD HH:mm:ss"
    />
  </el-form-item>
</template>

<script lang="ts">
import { ref } from 'vue';

interface FormValue {
  type: string;
  format?: string;
  default_value?: any; // Assuming this could be an ISO8601 string or similar
  show_default_value: boolean;
}

export default defineComponent({
  name: 'DateTimePickerFormItem',
  setup() {
    const formValue = ref<FormValue>({
      type: 'datetime',
      format: undefined,
      default_value: '',
      show_default_value: false,
    });

    const getData = () => {
      return {
        key: formValue.value.key, // Ensure the key exists in the original data structure
        ...formValue.value,
        format: formValue.value.format,
        'value-format': formValue.value.format,
      };
    };

    const rander = (form_data: any) => {
      formValue.value.type = form_data.attrs.type || '';
      formValue.value.format = form_data.attrs?.format ?? '';
      formValue.value.default_value = form_data.default_value || '';
      formValue.value.show_default_value = !!form_data.show_default_value;
    };

    defineExpose({ getData, rander });

    onBeforeMount(() => {});

    return {
      formValue,
    };
  },
});
</script>

<style lang="scss"></style>

Key Changes:

  • Property Consistency: Changed properties like default_value to use camelCase for JavaScript/TypeScript compatibility.
  • Added Comment: Explained the checkbox's purpose in the template slot content.
  • Type Annotation: Introduced TypeScript definitions (FormValue) for better clarity and error prevention.

}
defineExpose({ getData, rander })
onMounted(() => {
formValue.value.default_value = {}
formValue.value.show_default_value = true
})
</script>
<style lang="scss"></style>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code Review

The provided code snippet is for a Vue.js template with an ElFormItem component from Element Plus, which includes a JsonInput. Here's a review and some suggestions:

Regularities & Issues

  • Template: The template uses dynamic slots (#label) to conditionally display additional content before the label ("显示默认值" or "默认值").
  • Script:
    • getData Function: It initializes the default_value_rule, but it doesn't populate formValue.default_value correctly.
    • rander Function: This function attempts to update both formValue.default_value and show_default_value using data fetched elsewhere. However, it should only update what was passed by getData.
    • onMounted Hook: Initializes formValue.default_value to {}, but then immediately updates it again without fetching new data.

Optimization Suggestions

  1. Avoid Manual Initialization of Defaults: Ensure that default_value gets set correctly via the getData function. Currently, the first initialization inside onMounted doesn't seem necessary unless you have additional logic there.
  2. Remove Unnecessary Initializations: Remove the second initializations of default_value inside onMounted as they can be avoided if getDefaultData() actually fetches the correct values later on.
  3. Consistent Data Handling: In getData, consider handling all fields consistently. If some may not always return values, make sure they are properly nullified or handled instead of just being initialized.
  4. Rule Definition: Consider validating each field separately if there will be multiple rules defined in a global manner. Otherwise, consolidate them into individual rule objects where needed.
  5. Error Handling: Add some error handling around updating form values to ensure robustness against unexpected issues during rendering or API calls.

Revised Example Script

Here’s a revised version of the script considering my suggestions:

<script setup>
import { ref } from 'vue';
import JsonInput from './components/JsonInput.vue';

const formValue = ref({
  required: true,
  show_default_value: true,
});

// Dummy getData function simulating backend response
async function getData() {
  try {
    const responseData = await fetchData(); // Replace fetchData with actual API call
    Object.assign(formValue.value, responseData);
  } catch (error) {
    console.error('Failed to get data:', error);
  }
}

function rander(form_data: any) {
  // Overwrite current form value with received data
  Object.assign(formValue.value, form_data);

  // Optionally handle specific changes here if necessary
}

defineExpose({ getData, rander });

// Simulate fetching data after mount
onMounted(async () => {
  getData();
});
</script>

Ensure to replace fetchData() with your actual API request function to retrieve the necessary data. By focusing on consistency and removing unnecessary steps, this approach makes the code cleaner and more maintainable.

}

defineExpose({ getData, rander })
onMounted(() => {
formValue.value.option_list = []
formValue.value.default_value = ''
formValue.value.show_default_value = true

addOption()
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your rander function currently takes over setting the initial values of form properties and adding options to the list. This can be streamlined slightly without changing functionality.

Here are some suggestions:

  1. Consolidate Option List Reset: You can combine the resetting of option_list, default_value, and show_default_value into single lines in the onMounted hook.
  2. Optimize Data Initialization: Initialize default_value directly within rander instead of passing an empty string when it's not necessary to change the default value.

Here is an optimized version of your code:

const getData = () => {
  formData.option_list = formData.option_list || [];
}

// Simplify onMounted setup
onMounted(() => {
  formValue.value.option_list = [];
  // Avoid initializing show_default_value here or using formData.show_default_value because it is handled elsewhere
})

defineExpose({
  getData,
  rander
})

This way, you reduce unnecessary reassignments and streamline the initialization process.

@liuruibin liuruibin merged commit 8f00615 into main Dec 23, 2024
4 checks passed
@liuruibin liuruibin deleted the pr@main@refactor_default_value branch December 23, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants