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.

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

Jump to bottom

fix: [el-cascader] multiple search with v-model can't work #3350

Merged
merged 2 commits into from
Sep 14, 2021
Merged

fix: [el-cascader] multiple search with v-model can't work #3350

merged 2 commits into from
Sep 14, 2021

Conversation

Alanscut
Copy link
Contributor

@Alanscut Alanscut commented Sep 11, 2021 •
edited

fix: issue #3111
same fix with pr #3217

Please make sure these boxes are checked before submitting your PR, thank you!

  • Make sure you follow Element's contributing guide English | (中文 | Español | Français).
  • Make sure you are merging your commits to dev branch.
  • Add some descriptions and refer to relative issues for your PR.

@Alanscut Alanscut mentioned this pull request Sep 11, 2021
Closed
3 tasks
@sxzz sxzz linked an issue Sep 11, 2021 that may be closed by this pull request
[Bug Report] el-cascader multiple search with v-model can't work #3111
Closed
@element-bot
Copy link
Member

@shangxinbo

This comment was marked as abuse.

Sign in to view
@sxzz
Copy link
Collaborator

sxzz commented Sep 11, 2021

When will it be released？1.1.0-beta10?

We're working on docs website now. We will review and merge it ASAP.

@shangxinbo

This comment was marked as abuse.

Sign in to view
@sxzz
Copy link
Collaborator

sxzz commented Sep 12, 2021 •
edited

It works, but I don't understand the code you wrote at all. It is worth noting that config is a ComputedRef, so Object.keys(configTemp) never includes modelValue.
And I changed config to config.value, but the bug still exists.
WDYT?

@Alanscut
Copy link
Contributor Author

It works, but I don't understand the code you wrote at all. It is worth noting that config is a ComputedRef, so Object.keys(configTemp) never includes modelValue.
And I changed config to config.value, but the bug still exists.
WDYT?

The reason for this issue is that due to the change of modelvalue, the tree node is re-rendered and the uid changes at the same time. Because the filter list searched by the input text still uses the old node uid, all the nodes in filtered list cannot be selected after the first click.
This pr mainly excludes the re-rendering of nodes caused by modelvalue changes, so use configTemp and configCheck without modelvalue for comparison.
这个issue产生的原因就是由于modelvalue的改变，导致重新渲染树节点同时uid发生改变，由于输入文字搜索到的过滤列表使用的还是旧的节点uid，所以第一次点击之后无法选中。
这个pr主要就是排除modelvalue改变所导致的节点重新渲染，所以使用除去modelvalue的configTemp和configCheck 进行比较

sxzz
sxzz reviewed Sep 13, 2021
View reviewed changes
packages/components/cascader-panel/src/index.vue Outdated
lazyLoad(null, () => {
initialLoaded = true
const configTemp = Object.assign(config)
if (Object.keys(configTemp).includes('modelValue')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line always return false. Please check again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes，This if is not necessary 。i removed it

@shangxinbo

This comment was marked as abuse.

Sign in to view
@sxzz
Copy link
Collaborator

sxzz commented Sep 14, 2021

I modified the name of the variable and added the type.

@sxzz sxzz changed the title fix: el-cascader multiple search with v-model can't work fix: [el-cascader] multiple search with v-model can't work Sep 14, 2021
@sxzz sxzz merged commit 5978ebd into element-plus:dev Sep 14, 2021
@element-bot element-bot mentioned this pull request Sep 15, 2021
Merged
3 tasks
jw-foss added a commit that referenced this pull request Sep 15, 2021
@jw-foss @sxzz @btea
D2M (#3418)
136d5bc 
* fix release scirpt (#3410)

* fix release scirpt (#3412)

* fix: [el-cascader] multiple search with v-model can't work (#3350)

Co-authored-by: Kevin <sxzz@sxzz.moe>

* build: external packages (#3415)

* fix(utils): add type remove the ts error (#3406)

* fix(utils): add type remove the ts error

* fix: optional chaining operator replace non-nullish assertion

* chore: try again test

Co-authored-by: Alan Wang <948467222@qq.com>
Co-authored-by: Kevin <sxzz@sxzz.moe>
Co-authored-by: btea <2356281422@qq.com>
@element-bot element-bot mentioned this pull request Sep 22, 2021
Merged
3 tasks
@SimonaliaChen SimonaliaChen mentioned this pull request Sep 24, 2021
Closed
3 tasks
Alanscut added a commit to Alanscut/element-plus that referenced this pull request Sep 24, 2021
@Alanscut
fix: remove change of cascader in element-plus#3350 and edit guide of…
a93d0e2 
… props.multiple
Alanscut pushed a commit to Alanscut/element-plus that referenced this pull request Sep 26, 2021
Revert "fix: [el-cascader] multiple search with v-model can't work (e…
718b900 
…lement-plus#3350)"

This reverts commit 5978ebd.
@ghost ghost mentioned this pull request Jun 27, 2023
Closed
3 tasks
@ghost ghost mentioned this pull request Aug 2, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sxzz sxzz sxzz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug Report] el-cascader multiple search with v-model can't work
4 participants
@Alanscut @element-bot @shangxinbo @sxzz