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: Trigger onFocus/onBlur #425

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: Trigger onFocus/onBlur #425

wants to merge 3 commits into from

Conversation

lmotioc
Copy link

@lmotioc lmotioc commented Sep 16, 2020

What does it do?

Updates onFocus and onBlur events for the whole component to act like a composite input.
Triggers onFocus on first input focus of the select component and onBlur on last input blur of the select component.

Fixes #418

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • [n/a] Updated documentation (if applicable)
  • [n/a] Added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • My changes generate no new warnings

@coveralls
Copy link

coveralls commented Sep 16, 2020

Pull Request Test Coverage Report for Build 1505

  • 19 of 22 (86.36%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.3%) to 93.672%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/index.js 19 22 86.36%
Totals Coverage Status
Change from base Build 1495: -0.3%
Covered Lines: 604
Relevant Lines: 627

💛 - Coveralls

apa47
apa47 previously approved these changes Sep 16, 2020
@lmotioc
Copy link
Author

lmotioc commented Sep 16, 2020

i'm not sure i should refactor the main component to fix the codeclimate issue. any advice @mrchief sir? 😁

onBlur = () => {
// setTimeout runs afterwards in the event.
// If onFocus is triggered immediately in a child component, clearTimeout will stop setTimeout to run.
this._timeoutID = setTimeout(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

setTimeout/clearTimeout seems like a fragile solution. Not just from a timing perspective but I think it makes it harder to move towards hooks. I was going to ask if you have you looked at using requestAnimationFrame in lieu of setTimeout but then I see that it's being used in conjunction with clearTimeout so RAF might not be an option.

I'm curious though - why take on the additional burden of keeping track of things instead of detecting outside clicks?

Copy link
Author

Choose a reason for hiding this comment

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

I agree it looks a bit like smelly code, but it does not delay in terms of time but on one cycle on the event loop.

I choose this solution, first, because it builds over the existing onFocus/blur events from the dom and it doesn't need to treat corner cases. in my opinion this is easier to maintain then associating the blur focus with other events like opening and closing the dropdown. Secondly, the show stopper for me was the complexity of having the blur triggered when using keyboard navigation in a form. you could be listening on tab on the last element but you can not be sure which one is it,you have to check the props. it seemed to add to much complexity on the code.

i also tried attaching a handler like it is done for closing the dropdown when clicking outside but i ran into some corner cases, and i am sure i cannot think of all of them. also it might be harder to maintain.

if (nextFocus) {
nextFocus.focus()
} else {
this.onBlur()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may not be an onBlur as the user would still be in the input, wouldn't they?

Copy link
Author

Choose a reason for hiding this comment

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

i can not see a case where the component is on focus. if the focused element is removed, and there is nothing else you can focus on, doesn't that mean the component loses focus? what am i missing?

@@ -114,6 +114,7 @@ declare module 'react-dropdown-tree-select' {
searchInput: HTMLInputElement
keepDropdownActive: boolean
handleClick(): void
_timeoutID: number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to expose this?

@codeclimate
Copy link

codeclimate bot commented Sep 16, 2020

Code Climate has analyzed commit 862180d and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@stale
Copy link

stale bot commented Nov 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 8, 2020
@mrchief mrchief added wip Work In Progress and removed stale labels Nov 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wip Work In Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

onBlur has inconsistent behavior.
4 participants