Skip to content

Conversation

@daviForevel
Copy link
Collaborator

No description provided.

@github-actions
Copy link

PR preview has been successfully built and deployed to https://vue-devui-pr-1128.surge.sh.

@kagol kagol added the refactoring Refactor code label Jul 28, 2022
@github-actions
Copy link

PR preview has been successfully built and deployed to https://vue-devui-pr-1128.surge.sh.

@github-actions
Copy link

PR preview has been successfully built and deployed to https://vue-devui-pr-1128.surge.sh.

@github-actions
Copy link

PR preview has been successfully built and deployed to https://vue-devui-pr-1128.surge.sh.

Copy link
Collaborator

@TerminatorSd TerminatorSd left a comment

Choose a reason for hiding this comment

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

检视use-select.ts重构

} else {
checkedItems.push(item.value);
}
modelValue = checkedItems;
Copy link
Collaborator

Choose a reason for hiding this comment

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

props的数据流应该是单向流动的,在子组件中修改父元素props的值,会报warning吧?

const { filterQuery, isSupportFilter, injectOptionsArray, t } = option;

// no-data-text
const isLoading = computed(() => typeof props.loading === 'boolean' && props.loading);
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里建议不对props.loading进行是否为boolean的检查,可以消除isLoading中间变量,直接使用props.loading。可以讨论一下,当用户传入的loading值为一个非空的字符串或者数字的时候,我们是否希望loading是工作的?

// no-data-text
const isLoading = computed(() => typeof props.loading === 'boolean' && props.loading);
const emptyText = computed(() => {
const visibleOptionsCount = injectOptionsArray.value.filter((item) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

visibleOptionsCount主要是用来判断是否存在visibleOptions,所以是否可以将名字改为hasVisibleOptions,同时后面的filter方法替换为some方法。

});

const isShowEmptyText = computed(() => {
return !!emptyText.value && (!props.allowCreate || isLoading.value || (props.allowCreate && injectOptionsArray.value.length === 0));
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里的判断条件有些复杂,存在&&与||的嵌套,建议填加注释,不然比较难读懂。

// allow-create
const isShowCreateOption = computed(() => {
const hasCommonOption = injectOptionsArray.value.filter((item) => !item.create).some((item) => item.name === filterQuery.value);
return props.filter === true && props.allowCreate && !!filterQuery.value && !hasCommonOption;
Copy link
Collaborator

Choose a reason for hiding this comment

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

判断条件较为复杂,建议添加注释。

filterQuery.value = '';
}
if (isSupportFilter.value) {
focus();
Copy link
Collaborator

Choose a reason for hiding this comment

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

focus方法在当前文件没有找到声明的位置,此处是如何工作的?

@kagol
Copy link
Member

kagol commented Aug 5, 2022

@daviForevel 有冲突和检视意见未解决

@kagol
Copy link
Member

kagol commented Sep 11, 2022

@daviForevel 这个PR的冲突和检视意见太久未解决,先关闭了

@kagol kagol closed this Sep 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants