fix: Trigger onFocus/onBlur #425
Conversation
Pull Request Test Coverage Report for Build 1505
💛 - Coveralls |
|
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(() => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
This may not be an onBlur as the user would still be in the input, wouldn't they?
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
Why do we need to expose this?
|
Code Climate has analyzed commit 862180d and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
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. |
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
Checklist: