feat: Support single line multi select cards#3976
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 |
| :deep(.el-select-dropdown) { | ||
| max-width: 400px; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
The provided code for a Vue.js component to render a dynamic form is mostly well-written. Here are some considerations for improvement and potential optimizations:
-
Code Duplicacy: There's significant duplicated logic in handling the
assignment_methodselection, especially when setting updefault_ref_variables_value_rule. Consider abstracting this into reusable functions. -
Error Handling: The function that validates the
ref_variablesvalues does not handle cases where the first item of the validation array is incorrect. Consider adding early returns or more robust error messages. -
Performance Optimization: The use of deep selectors like
:deep()can be overkill, particularly if other components might also use these styles in unintended ways. It would be better to isolate specific classes within their respective components. -
Consistent Formatting: Ensure consistent spacing and line breaks within comments and strings for easier readability.
-
Event Emission: Although not explicitly shown, ensure there’s no missing events being emitted that could affect behavior elsewhere in the application.
Here is a revised version with suggested improvements:
<script setup lang="ts">
import { computed, onMounted, inject } from 'vue';
import MultiRow from '@/components/dynamics-form/items/MultiRow.vue';
import NodeCascader from '@/workflow/common/NodeCascader.vue';
import type { FormField } from '@/components/dynamics-form/type';
const getModel = inject('getModel') as any;
// Abstracted function for validating default_ref_variables_value_rule
function validateRefVariables(rule: any, value: any): boolean {
if (!Array.isArray(value) || value.length <= 1) {
return false;
}
// Additional custom validations if needed
return true;
}
const assignment_method_option_list = computed(() => {
const option_list = [
{
label: t('dynamicsForm.AssignmentMethod.custom.label', '自定义'),
value: 'custom',
},
];
if (getModel) {
option_list.push({
label: t('dynamicsForm.AssignmentMethod.ref_variables.label', '引用变量'),
value: 'ref_variables',
});
}
return option_list;
});
... rest of the script remains the same
</script>
<style scoped>
.defaultValueItem {
position: relative;
.defaultValueCheckbox {
position: absolute;
right: 0;
top: -35px;
}
}
/* Isolated deeper selector */
/deep/.el-select-dropdown {
max-width: 400px;
}
</style> These adjustments improve the maintainability and consistency of the code while ensuring functionality remains intact.
| } | ||
| } | ||
| } | ||
| </style> |
There was a problem hiding this comment.
The code looks mostly clean, but here are a few recommendations:
-
Avoid
void 0injection: Instead of injecting intoformItemContextKey, directly use the injected function to access the validation method. -
Use concise template expression syntax: This makes the code more readable and maintainable.
-
Refactor conditional classes: Use CSS variables instead of hardcoded colors for better flexibility.
Here's how you could refactor it:
<template>
<div class="radio_content">
<div
v-for="item in option_list"
:key="item[value_field]"
class="item"
:class="{ is_disabled: !inputDisabled, [activeClass]: isActive(item) }"
@click="selected(item[value_field])"
>
{{ item[textField] }}
</div>
</div>
</template>
<script lang="ts" setup>
import { computed, inject } from 'vue';
import type { FormField } from '@/components/dynamics-form/type';
import formItemContextKey from 'element-plus/es/components/form/src/constants';
const inputDisabled = inject(formItemContextKey);
defineProps<{
formValue?: any;
formffieldList?: Array<FormField>;
field: string;
otherParams: any;
formField: FormField;
view?: boolean;
modelValue?: any;
}>();
const elFormItem = inject(formItemContextKey) as unknown as ElFormComponent;
function selected(activeValue: string | number) {
const newValue = props.modelValue || [];
if (!newValue.includes(activeValue)) {
newValue.push(activeValue);
} else {
const index = newValue.findIndex(i => i === activeValue);
newValue.splice(index, 1);
}
emit('update:modelValue', newValue);
if (elFormItem && elFormItem.validate) {
elFormItem.clearValidate();
}
}
const textField = computed(() => props.formField.text_field || 'key');
const valueField = computed(() => props.formField.value_field || 'value');
const option_list = computed(() => props.formField.option_list || []);
function isActive(item: { [key: string]: string }) {
return isActiveComputed().includes(item[value_field]);
}
// Calculate the active state based on modelValue
const isActiveComputed = computed(() => props.modelValue || []);
</script>
<style lang="scss" scoped>
.radio_content {
$border-color: #bbbfc4;
$primary-bg-light-9: lighten(#57aaff, 5%);
$disabled-bg: lighten(#ccc, 6%);
height: 32px;
display: inline-flex;
border: 1px solid $border-color;
border-radius: 4px;
font-weight: 400;
font-size: 14px;
color: var(--el-text-color-primary);
padding: 3px 4px;
box-sizing: border-box;
white-space: nowrap;
&.is-disabled {
border: 1px solid $disable-bg;
background: $disabled-bg;
color: var(--el-text-color-placeholder);
cursor: not-allowed;
&:hover {
cursor: not-allowed;
}
}
.active {
border-radius: 4px;
background: $primary-bg-light-9;
color: #57aaff;
}
.item {
cursor: pointer;
margin: 0 2px;
padding: 2px 8px;
height: 20px;
display: flex;
justify-content: center;
align-items: center;
&:last-child {
margin: 0 4px 0 2px;
}
&:first-child {
margin: 0 2px 0 4px;
}
}
}
</style>This refactoring improves readability and simplifies some logic while using modern Vue.js best practices.
| if ['SingleSelect', 'MultiSelect', 'RadioCard', 'RadioRow', 'MultiRow'].__contains__(field.get('input_type')): | ||
| if field.get('assignment_method') == 'ref_variables': | ||
| option_list = self.workflow_manage.get_reference_field(field.get('option_list')[0], | ||
| field.get('option_list')[1:]) |
There was a problem hiding this comment.
There are no significant irregularities or potential issues in this code. However, there are a few minor optimizations that can be made:
-
Replace
list.__contains__withinoperator in bothifstatements to improve readability. -
Move the declaration of
multi_select_listoutside the functions and define it at class level, where applicable, especially if used frequently across multiple methods. This will make the function more concise and reusable. -
Consider using a set instead of list for
multi_select_listsince sets offer O(1) average time complexity for membership checks.
Here's an example of how you could refactor these changes:
# Define multi-select options as a set for faster membership-checking
multi_select_set = {'.MultiSelect', 'MultiRow'}
def get_default_option(option_list, _type, value_field):
# ... rest of the code remains similar ...
def reset_field(self, field):
# ... rest of the code remains similar ...These small improvements enhance readability and maintainability while keeping the logic essentially unchanged.
feat: Support single line multi select cards