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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use onClick handler on items rather than on root #192

Merged
merged 1 commit into from
Sep 20, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 0 additions & 2 deletions src/__tests__/__snapshots__/downshift.get-root-props.js.snap
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`not applying the onClick prop results in an error 1`] = `"downshift: You must apply the \\"onClick\\" prop from getRootProps onto your root element."`;

exports[`not applying the ref prop results in an error 1`] = `"downshift: You must apply the ref prop \\"ref\\" from getRootProps onto your root element."`;

exports[`returning a DOM element and calling getRootProps with a refKey results in an error 1`] = `"downshift: You returned a DOM element. You should not specify a refKey in getRootProps. You specified \\"blah\\""`;
Expand Down
12 changes: 0 additions & 12 deletions src/__tests__/downshift.get-root-props.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,18 +47,6 @@ test('not applying the ref prop results in an error', () => {
expect(() => mount(<MyComponent />)).toThrowErrorMatchingSnapshot()
})

test('not applying the onClick prop results in an error', () => {
const MyComponent = () => (
<Downshift>
{({getRootProps}) => {
const {ref} = getRootProps()
return <div ref={ref} />
}}
</Downshift>
)
expect(() => mount(<MyComponent />)).toThrowErrorMatchingSnapshot()
})

test('renders fine when rendering a composite component and applying getRootProps properly', () => {
const MyComponent = () => (
<Downshift>
Expand Down
39 changes: 5 additions & 34 deletions src/downshift.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,11 @@ import PropTypes from 'prop-types'
import setA11yStatus from './set-a11y-status'
import {
cbToCb,
findParent,
composeEventHandlers,
debounce,
scrollIntoView,
generateId,
firstDefined,
isNumber,
getA11yStatusMessage,
unwrapArray,
isDOMElement,
Expand Down Expand Up @@ -370,35 +368,17 @@ class Downshift extends Component {

rootRef = node => (this._rootNode = node)

getRootProps = ({refKey = 'ref', onClick, ...rest} = {}) => {
getRootProps = ({refKey = 'ref', ...rest} = {}) => {
// this is used in the render to know whether the user has called getRootProps.
// It uses that to know whether to apply the props automatically
this.getRootProps.called = true
this.getRootProps.refKey = refKey
return {
[refKey]: this.rootRef,
onClick: composeEventHandlers(onClick, this.root_handleClick),
...rest,
}
}

root_handleClick = event => {
event.preventDefault()
const itemParent = findParent(
node => {
const index = this.getItemIndexFromId(node.getAttribute('id'))
return isNumber(index)
},
event.target,
this._rootNode,
)
if (itemParent) {
this.selectItemAtIndex(
this.getItemIndexFromId(itemParent.getAttribute('id')),
)
}
}

//\\\\\\\\\\\\\\\\\\\\\\\\\\ ROOT

keyDownHandlers = {
Expand Down Expand Up @@ -566,17 +546,10 @@ class Downshift extends Component {
return `${this.id}-item-${index}`
}

getItemIndexFromId(id) {
if (id) {
return Number(id.split(`${this.id}-item-`)[1])
} else {
return null
}
}

getItemProps = (
{
onMouseEnter,
onClick,
index,
item = requiredProp('getItemProps', 'item'),
...rest
Expand All @@ -595,6 +568,9 @@ class Downshift extends Component {
type: Downshift.stateChangeTypes.itemMouseEnter,
})
}),
onClick: composeEventHandlers(onClick, () => {
this.selectItemAtIndex(index)
}),
...rest,
}
}
Expand Down Expand Up @@ -766,9 +742,4 @@ function validateGetRootPropsCalledCorrectly(element, {refKey}) {
`downshift: You must apply the ref prop "${refKey}" from getRootProps onto your root element.`,
)
}
if (!getElementProps(element).hasOwnProperty('onClick')) {
throw new Error(
`downshift: You must apply the "onClick" prop from getRootProps onto your root element.`,
)
}
}
10 changes: 1 addition & 9 deletions src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,6 @@ function firstDefined(...args) {
return args.find(a => typeof a !== 'undefined')
}

function isNumber(thing) {
// not NaN and is a number type
// eslint-disable-next-line no-self-compare
return thing === thing && typeof thing === 'number'
}

// eslint-disable-next-line complexity
function getA11yStatusMessage({
isOpen,
Expand Down Expand Up @@ -242,7 +236,7 @@ const stateKeys = [
*/
function pickState(state = {}) {
const result = {}
stateKeys.forEach((k) => {
stateKeys.forEach(k => {
if (state.hasOwnProperty(k)) {
result[k] = state[k]
}
Expand All @@ -252,13 +246,11 @@ function pickState(state = {}) {

export {
cbToCb,
findParent,
composeEventHandlers,
debounce,
scrollIntoView,
generateId,
firstDefined,
isNumber,
getA11yStatusMessage,
unwrapArray,
isDOMElement,
Expand Down