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

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

fix: Trigger onFocus/onBlur #425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39cc255
fix: Trigger onFocus/onBlur
lmotioc Sep 16, 2020
ac42d5d
fix: Update tests/story
lmotioc Sep 16, 2020
862180d
fix: Removed public export for timeoutId
lmotioc Sep 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions __snapshots__/src/index.test.js.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ Generated by [AVA](https://ava.li).
<div
className="react-dropdown-tree-select"
id="rdts"
onBlur={Function {}}
onFocus={Function {}}
>
<div
className="dropdown"
Expand Down Expand Up @@ -180,6 +182,8 @@ Generated by [AVA](https://ava.li).
<div
className="react-dropdown-tree-select"
id="rdts"
onBlur={Function {}}
onFocus={Function {}}
>
<div
className="dropdown"
Expand Down Expand Up @@ -348,6 +352,8 @@ Generated by [AVA](https://ava.li).
<div
className="react-dropdown-tree-select"
id="rdts"
onBlur={Function {}}
onFocus={Function {}}
>
<div
className="dropdown"
Expand Down Expand Up @@ -469,6 +475,8 @@ Generated by [AVA](https://ava.li).
<div
className="react-dropdown-tree-select"
id="rdts"
onBlur={Function {}}
onFocus={Function {}}
>
<div
className="dropdown radio-select"
Expand Down Expand Up @@ -620,6 +628,8 @@ Generated by [AVA](https://ava.li).
<div
className="react-dropdown-tree-select"
id="rdts"
onBlur={Function {}}
onFocus={Function {}}
>
<div
className="dropdown"
Expand Down Expand Up @@ -689,6 +699,8 @@ Generated by [AVA](https://ava.li).
<div
className="react-dropdown-tree-select"
id="rdts"
onBlur={Function {}}
onFocus={Function {}}
>
<div
className="dropdown"
Expand Down
Binary file modified __snapshots__/src/index.test.js.snap
View file
Open in desktop
Binary file not shown.
8 changes: 8 additions & 0 deletions docs/src/stories/Options/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ class WithOptions extends PureComponent {
onNodeToggle = curNode => {
console.log('onNodeToggle::', curNode)
}
onFocus = (node, action) => {
console.log('onFocus::', action, node)
}
onBlur = (node, action) => {
console.log('onBlur::', action, node)
}

onOptionsChange = value => {
this.setState({ [value]: !this.state[value] })
Expand Down Expand Up @@ -121,6 +127,8 @@ class WithOptions extends PureComponent {
id="rdts"
data={data}
onChange={this.onChange}
onBlur={this.onBlur}
onFocus={this.onFocus}
onAction={this.onAction}
onNodeToggle={this.onNodeToggle}
clearSearchOnChange={clearSearchOnChange}
Expand Down
71 changes: 48 additions & 23 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ class DropdownTreeSelect extends Component {
this.state = {
searchModeOn: false,
currentFocus: undefined,
isManagingFocus: false,
}
this.clientId = props.id || clientIdGenerator.get(this)
}
Expand Down Expand Up @@ -106,40 +107,59 @@ class DropdownTreeSelect extends Component {
}

componentWillUnmount() {
document.removeEventListener('click', this.handleOutsideClick, false)
document.removeEventListener('click', this.handleDropdownCollapse, false)
}

componentWillReceiveProps(nextProps) {
this.initNewProps(nextProps)
}

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 (this.state.isManagingFocus) {
this.props.onBlur()
this.setState({
isManagingFocus: false,
})
}
}, 0)
}

onFocus = () => {
clearTimeout(this._timeoutID)
if (!this.state.isManagingFocus) {
this.props.onFocus()
this.setState({
isManagingFocus: true,
})
}
}

handleClick = (e, callback) => {
this.setState(prevState => {
// keep dropdown active when typing in search box
const showDropdown = this.props.showDropdown === 'always' || this.keepDropdownActive || !prevState.showDropdown

// register event listeners only if there is a state change
if (showDropdown !== prevState.showDropdown) {
if (showDropdown) {
document.addEventListener('click', this.handleOutsideClick, false)
} else {
document.removeEventListener('click', this.handleOutsideClick, false)
}
const searchStateReset = !showDropdown ? this.resetSearchState() : {}
// adds event listener for collapsing the dropdown
if (this.state.isManagingFocus && this.props.showDropdown !== 'always') {
document.addEventListener('click', this.handleDropdownCollapse, false)
}

if (showDropdown) this.props.onFocus()
else this.props.onBlur()

return !showDropdown ? { showDropdown, ...this.resetSearchState() } : { showDropdown }
return {
showDropdown,
searchStateReset,
}
}, callback)
}

handleOutsideClick = e => {
if (this.props.showDropdown === 'always' || !isOutsideClick(e, this.node)) {
return
}

this.handleClick()
handleDropdownCollapse = e => {
if (!isOutsideClick(e, this.node)) return
document.removeEventListener('click', this.handleDropdownCollapse, false)
const showDropdown = this.props.showDropdown === 'always'
this.setState({ showDropdown: showDropdown })
}

onInputChange = value => {
Expand All @@ -157,12 +177,15 @@ class DropdownTreeSelect extends Component {
})
}

onTagRemove = (id, isKeyboardEvent) => {
onTagRemove = id => {
const { tags: prevTags } = this.state
this.onCheckboxChange(id, false, tags => {
if (!isKeyboardEvent) return

keyboardNavigation.getNextFocusAfterTagDelete(id, prevTags, tags, this.searchInput).focus()
const nextFocus = keyboardNavigation.getNextFocusAfterTagDelete(id, prevTags, tags, this.searchInput)
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?

}
})
}

Expand Down Expand Up @@ -201,7 +224,7 @@ class DropdownTreeSelect extends Component {
}

if (isSingleSelect && !showDropdown) {
document.removeEventListener('click', this.handleOutsideClick, false)
document.removeEventListener('click', this.handleDropdownCollapse, false)
}

keyboardNavigation.adjustFocusedProps(currentFocusNode, node)
Expand Down Expand Up @@ -311,6 +334,8 @@ class DropdownTreeSelect extends Component {
return (
<div
id={this.clientId}
onBlur={this.onBlur}
onFocus={this.onFocus}
className={[this.props.className && this.props.className, 'react-dropdown-tree-select']
.filter(Boolean)
.join(' ')}
Expand Down
9 changes: 5 additions & 4 deletions src/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,15 +243,16 @@ test('deactivates dropdown active on blur', t => {
test('detects click outside', t => {
const { tree } = t.context
const wrapper = mount(<DropdownTreeSelect data={tree} />)
const handleOutsideClick = spy(wrapper.instance(), 'handleOutsideClick')
const handleDropdownCollapse = spy(wrapper.instance(), 'handleDropdownCollapse')

wrapper.instance().onFocus()
wrapper.instance().handleClick()
t.true(wrapper.state().showDropdown)

const event = new MouseEvent('click', { bubbles: true, cancelable: true })
global.document.dispatchEvent(event)

t.true(handleOutsideClick.calledOnce)
t.true(handleDropdownCollapse.calledOnce)
t.false(wrapper.state().showDropdown)
})

Expand All @@ -270,7 +271,7 @@ test('detects click inside', t => {
target: checkboxItem,
})
Object.defineProperty(event, 'target', { value: checkboxItem, enumerable: true })
wrapper.instance().handleOutsideClick(event)
wrapper.instance().handleDropdownCollapse(event)

t.true(wrapper.state().showDropdown)
})
Expand All @@ -291,7 +292,7 @@ test('detects click outside when other dropdown instance', t => {
target: searchInput,
})
Object.defineProperty(event, 'target', { value: searchInput, enumerable: true })
wrapper1.instance().handleOutsideClick(event)
wrapper1.instance().handleDropdownCollapse(event)

t.false(wrapper1.state().showDropdown)
})
Expand Down