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

fix(directives): [repeat-click] Interval time is too short for single clicks #9466

Merged
merged 8 commits into from Aug 31, 2022

Conversation

opuu
Copy link
Contributor

@opuu opuu commented Aug 25, 2022

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

  • Make sure you follow 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.

The interval to detect long press (100ms) is too low, so single clicks are also being considered as repeat click.

For example: Clicking the plus or minus button of the Input Number component increases number by 2 instead of the defined step value (1).

multiple-click

This PR fixes the Input Number component.

@pull-request-triage
Copy link

👋 @opuu, seems like this is your first time contribution to element-plus.

  • Please make sure that you have read our guidelines and code of conduct before making a contribution.
  • You can comment with /label Components:[component_name] to add a label for which component you are working on.
  • You may join our Discord for staying tuned.

@pull-request-triage pull-request-triage bot added 1st contribution Their very first contribution Needs Review labels Aug 25, 2022
@github-actions
Copy link

github-actions bot commented Aug 25, 2022

@github-actions
Copy link

Hello @opuu, thank you for contributing to element-plus, please see our guideline to see how to make contribution

@github-actions
Copy link

github-actions bot commented Aug 26, 2022

🧪 Playground Preview: https://element-plus.run/?pr=9466
Please comment the example via this playground if needed.

@holazz
Copy link
Member

holazz commented Aug 26, 2022

Hi, maybe you need also to modify the test cases.

@opuu
Copy link
Contributor Author

opuu commented Aug 26, 2022

Hi, maybe you need also to modify the test cases.

Yes, making the sleep time 630 ms will fix it. I will do it soon.

@opuu
Copy link
Contributor Author

opuu commented Aug 26, 2022

@holazz fixed the unit test issue, the current failed test does not seems to be related to this PR.

Edit: It is related. Working on it.

@opuu
Copy link
Contributor Author

opuu commented Aug 26, 2022

Now it should pass all tests 🧪

@opuu
Copy link
Contributor Author

opuu commented Aug 26, 2022

@holazz It is related to this PR. The up and down arrows are using repeat click directive in Time Picker which also registers single click as double clicks.

@holazz
Copy link
Member

holazz commented Aug 26, 2022

@holazz It is related to this PR. The up and down arrows are using repeat click directive in Time Picker which also registers single click as double clicks.

You're right! I missed it.😅

@holazz
Copy link
Member

holazz commented Aug 26, 2022

@opuu Thank you for your contribution. After a little thought, I think a more reasonable way is not to increase the time interval of the long press trigger event, but to delay the trigger of the first event after the long press. I added a commit to this PR. What do you think?

Before

2022-08-26.5.03.29.mov

After

2022-08-26.5.14.31.mov

@opuu
Copy link
Contributor Author

opuu commented Aug 26, 2022

@opuu Thank you for your contribution. After a little thought, I think a more reasonable way is not to increase the time interval of the long press trigger event, but to delay the trigger of the first event after the long press. I added a commit to this PR. What do you think?

Thats even better 👌 This way it will behave more like HTML input with number type.

@holazz holazz requested a review from a team August 26, 2022 09:19
@holazz
Copy link
Member

holazz commented Aug 29, 2022

Would you mind taking a look on this? @buqiyuan @tolking

@xiaoxian521 xiaoxian521 self-requested a review August 31, 2022 05:47
@xiaoxian521 xiaoxian521 merged commit e25df54 into element-plus:dev Aug 31, 2022
@element-bot element-bot mentioned this pull request Sep 2, 2022
3 tasks
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.

None yet

4 participants