Skip to content

Autocomplete: add clearable prop #12171

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

Merged
merged 3 commits into from
Nov 13, 2018

Conversation

arthurdenner
Copy link
Contributor

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

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

This PR allows el-autocomplete to be clearable, just like el-input.

@element-bot
Copy link
Member

element-bot commented Jul 31, 2018

Deploy preview for element ready!

Built with commit 88ff1b7

https://deploy-preview-12171--element.netlify.com

@ziyoung
Copy link
Contributor

ziyoung commented Jul 31, 2018

@arthurdenner it seems that you only add clearable attribute and this attribute does'nt work.

@arthurdenner
Copy link
Contributor Author

@ziyoung maybe I'm missing something, but the props object is passed to el-input here, so I just need to declare it as a prop. Testing locally, if I put the default value as true, it works as expected.

@ziyoung
Copy link
Contributor

ziyoung commented Jul 31, 2018

@arthurdenner after I click the clear icon, the autocomplete suggestions are expected to display all.
autocomplete

@arthurdenner
Copy link
Contributor Author

arthurdenner commented Jul 31, 2018

Sorry @ziyoung, I didn't notice this. Thanks for pointing it out.

I did a little digging, tried a few things, but only got it fixed by adding @clear="handleClear" on el-input.

handleClear() {
  this.$nextTick(_ => {
    this.handleFocus();
  });
},

However, I'm not sure if that's the best solution so I didn't commit it. Would you accept this as a solution or do you have an idea of how to fix it properly?

@wacky6
Copy link
Contributor

wacky6 commented Jul 31, 2018

My recommendation is "try to avoid $nextTick / setImmediate / setTimeout", they could cause timing issues, and they don't offer a clear picture of function call stack and state transition.

@arthurdenner
Copy link
Contributor Author

Thanks, @wacky6. I'm aware of that, which is why I'm not satisfied with this fix.

But what I noticed is: though focus is only called after emitting the clear event, the suggestions are re-calculated before el-autocomplete realizes that its value is an empty string, so it re-calculates with the wrong this.value.

Do you have a suggestion to a proper fix?

@wacky6
Copy link
Contributor

wacky6 commented Jul 31, 2018

Why not just call auto-complete's handleFocus or debouncedGetData?

You might also need to consider emission of 'focus' event. Personally, I don't think clearing auto-complete should emit another focus event.

@arthurdenner
Copy link
Contributor Author

Good point, it shouldn't emit another focus event. Calling this.debouncedGetData(this.value) directly on handleClear doesn't work (for the same reason described above). It only works if called in the nextTick callback.

@wacky6
Copy link
Contributor

wacky6 commented Aug 1, 2018

I've looked at the source code, the problem is more serious than I thought.

  • el-input focuses itself after being cleared. This emits a 'focus' event, which is handled in auto-complete, at this instant, auto-complete's value has not been updated. handleFocus then picks up the old value, thus the dropdown list is incorrect.
  • el-input's behavior of focusing itself is inconsistent with other components. Select, Cascader, DatePicker will not focus themselves after being cleared.

For reference, input's clearable was added in #8509 (community contribution)


Consider:

  • A quick fix: I would consider adding a flag to "ignore" focus event once after being cleared (the current activated prop may be useful). Or as you suggested, use $nextTick.
  • A more complete fix: make el-input not to focus itself after being cleared, and autocomplete should close itself after being cleared. (to stay consistent with select/cascader/date-picker). This could be considered to be incompatible with previous releases.

@jikkai @ziyoung Please review the proposed fixes.

@arthurdenner
Copy link
Contributor Author

@wacky6, just so you know, I've implemented the more complete fix but I'm waiting for the fixes you proposed to be reviewed. If you think this could be a breaking change, I can change the pull request to the next branch instead of dev.

@JasonZough
Copy link

hope this will soon be released !

@arthurdenner
Copy link
Contributor Author

@wacky6, I committed the more complete fix and changed the PR to the next branch.

@arthurdenner arthurdenner changed the base branch from dev to next August 17, 2018 12:47
@wacky6
Copy link
Contributor

wacky6 commented Aug 18, 2018

LGTM for next. Should add some tests afterwards.

jsfiddle playground: https://jsfiddle.net/zd6gfbn1/

@jikkai jikkai force-pushed the next branch 2 times, most recently from f6f51ae to ef33295 Compare August 28, 2018 09:18
@arthurdenner
Copy link
Contributor Author

Sorry for bothering, but what is missing for this PR to be accepted? 😕

@wacky6 wacky6 added this to the 3.0 milestone Aug 30, 2018
@wacky6
Copy link
Contributor

wacky6 commented Aug 30, 2018

@jikkai This PR can be merged for 3.0 milestone

@arthurdenner
Copy link
Contributor Author

Hi guys, any updates about this PR? I'm about to launch a product and I would like to have this feature. :/

@wacky6
Copy link
Contributor

wacky6 commented Nov 10, 2018

@ziyoung Should this PR go to 2.0 or 3.0?

@ziyoung
Copy link
Contributor

ziyoung commented Nov 12, 2018

@arthurdenner clear event is added in pr #13326. I didn't notice there was a bug. It's my fault.☹️
We can merge this pr to 2.x.

@arthurdenner arthurdenner changed the base branch from next to master November 12, 2018 14:50
@arthurdenner arthurdenner force-pushed the autocomplete/clearable-prop branch from 0890304 to 88ff1b7 Compare November 12, 2018 14:53
@arthurdenner arthurdenner changed the base branch from master to dev November 12, 2018 14:57
@arthurdenner
Copy link
Contributor Author

@ziyoung I've rebased the branch and the PR points to the dev branch now.
Let me know if I need to do anything before this get merged please.

@ziyoung
Copy link
Contributor

ziyoung commented Nov 13, 2018

@arthurdenner done.😄

@ziyoung ziyoung merged commit 0265586 into ElemeFE:dev Nov 13, 2018
@arthurdenner
Copy link
Contributor Author

arthurdenner commented Nov 15, 2018

Thank you very much, @ziyoung!

@arthurdenner arthurdenner deleted the autocomplete/clearable-prop branch November 15, 2018 08:33
weisiren168 pushed a commit to weisiren168/element that referenced this pull request Jun 20, 2019
* Autocomplete: add clearable prop

* Input: remove focus after clearing the value

* Autocomplete: hide options after clearing the value
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.

5 participants