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

Validate that getMenuProps has set the ref correctly #525

Merged
merged 3 commits into from
Jul 29, 2018
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
17 changes: 15 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,7 @@ overridden (or overriding the props returned). For example:
| `getInputProps` | `function({})` | returns the props you should apply to the `input` element that you render. |
| `getItemProps` | `function({})` | returns the props you should apply to any menu item elements you render. |
| `getLabelProps` | `function({})` | returns the props you should apply to the `label` element that you render. |
| `getMenuProps` | `function({})` | returns the props you should apply to the `ul` element (or root of your menu) that you render. |
| `getMenuProps` | `function({},{})` | returns the props you should apply to the `ul` element (or root of your menu) that you render. |
| `getRootProps` | `function({},{})` | returns the props you should apply to the root element that you render. It can be optional. |

#### `getRootProps`
Expand Down Expand Up @@ -627,6 +627,13 @@ This method should be applied to the element which contains your list of items.
Typically, this will be a `<div>` or a `<ul>` that surrounds a `map` expression.
This handles the proper ARIA roles and attributes.

Required properties:

- `refKey`: if you're rendering a composite component, that component will need
to accept a prop which it forwards to the root DOM element. Commonly, folks
call this `innerRef`. So you'd call: `getMenuProps({refKey: 'innerRef'})` and
your composite component would forward like: `<ul ref={props.innerRef} />`

Optional properties:

- `aria-label`: By default the menu will add an `aria-labelledby` that refers
Expand All @@ -635,6 +642,12 @@ Optional properties:
available, then `aria-labelledby` will not be provided and screen readers
can use your `aria-label` instead.

In some cases, you might want to completely bypass the `refKey` check. Then you
can provide the object `{suppressRefError : true}` as the second argument to
`getMenuProps`.
**Please use it with extreme care and only if you are absolutely sure that the ref
is correctly forwarded otherwise `Downshift` will unexpectedly fail.**

```jsx
<ul {...getMenuProps()}>
{!isOpen
Expand Down Expand Up @@ -912,7 +925,7 @@ Check out these examples of more advanced use/edge cases:

**Old Examples exist on [codesandbox.io][examples]:**

*🚨 This is a great contribution opportunity!* These are examples that have not yet been migrated to
_🚨 This is a great contribution opportunity!_ These are examples that have not yet been migrated to
[downshift-examples](https://codesandbox.io/s/github/kentcdodds/downshift-examples).
You're more than welcome to make PRs to the examples repository to move these examples over there.
[Watch this to learn how to contribute completely in the browser](https://www.youtube.com/watch?v=3PAQbhdkTtI&index=2&t=21s&list=PLV5CVI1eNcJgCrPH_e6d57KRUTiDZgs0u)
Expand Down
5 changes: 5 additions & 0 deletions src/__tests__/__snapshots__/downshift.get-menu-props.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`not applying the ref prop results in an error 1`] = `"downshift: The ref prop \\"ref\\" from getMenuProps was not applied correctly on your menu element."`;

exports[`using a composite component and calling getMenuProps without a refKey results in an error 1`] = `"downshift: The ref prop \\"ref\\" from getMenuProps was not applied correctly on your menu element."`;
114 changes: 114 additions & 0 deletions src/__tests__/downshift.get-menu-props.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import React from 'react'
import {render} from 'react-testing-library'
import Downshift from '../'

const oldError = console.error

beforeEach(() => {
console.error = jest.fn()
})

afterEach(() => {
console.error = oldError
})

const Menu = ({innerRef, ...rest}) => <div ref={innerRef} {...rest} />

test('using a composite component and calling getMenuProps without a refKey results in an error', () => {
const MyComponent = () => (
<Downshift
children={({getMenuProps}) => (
<div>
<Menu {...getMenuProps()} />
</div>
)}
/>
)
expect(() => render(<MyComponent />)).toThrowErrorMatchingSnapshot()
})

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

test('renders fine when rendering a composite component and applying getMenuProps properly', () => {
const MyComponent = () => (
<Downshift
children={({getMenuProps}) => (
<div>
<Menu {...getMenuProps({refKey: 'innerRef'})} />
</div>
)}
/>
)
expect(() => render(<MyComponent />)).not.toThrow()
})

test('using a composite component and calling getMenuProps without a refKey does not result in an error if suppressRefError is true', () => {
const MyComponent = () => (
<Downshift
children={({getMenuProps}) => (
<div>
<Menu {...getMenuProps({}, {suppressRefError: true})} />
</div>
)}
/>
)
expect(() => render(<MyComponent />)).not.toThrow()
})

test('returning a DOM element and calling getMenuProps with a refKey does not result in an error if suppressRefError is true', () => {
const MyComponent = () => (
<Downshift
children={({getMenuProps}) => (
<div>
<ul {...getMenuProps({refKey: 'blah'}, {suppressRefError: true})} />
</div>
)}
/>
)
expect(() => render(<MyComponent />)).not.toThrow()
})

test('not applying the ref prop results in an error does not result in an error if suppressRefError is true', () => {
const MyComponent = () => (
<Downshift
children={({getMenuProps}) => {
getMenuProps({}, {suppressRefError: true})
return (
<div>
<ul />
</div>
)
}}
/>
)
expect(() => render(<MyComponent />)).not.toThrow()
})

test('renders fine when rendering a composite component and applying getMenuProps properly even if suppressRefError is true', () => {
const MyComponent = () => (
<Downshift
children={({getMenuProps}) => (
<div>
<Menu
{...getMenuProps({refKey: 'innerRef'}, {suppressRefError: true})}
/>
</div>
)}
/>
)
expect(() => render(<MyComponent />)).not.toThrow()
})
37 changes: 36 additions & 1 deletion src/downshift.js
Original file line number Diff line number Diff line change
Expand Up @@ -780,7 +780,14 @@ class Downshift extends Component {

menuRef = node => (this._menuNode = node)

getMenuProps = ({refKey = 'ref', ref, ...props} = {}) => {
getMenuProps = (
{refKey = 'ref', ref, ...props} = {},
{suppressRefError = false} = {},
) => {
this.getMenuProps.called = true
this.getMenuProps.refKey = refKey
this.getMenuProps.suppressRefError = suppressRefError

return {
[refKey]: callAll(ref, this.menuRef),
role: 'listbox',
Expand Down Expand Up @@ -918,6 +925,14 @@ class Downshift extends Component {
}, 200)

componentDidMount() {
if (
this.getMenuProps.called &&
!this.getMenuProps.suppressRefError &&
/* istanbul ignore next (react-native) */ preval`module.exports = process.env.BUILD_REACT_NATIVE !== 'true'`
) {
validateGetMenuPropsCalledCorrectly(this._menuNode, this.getMenuProps)
}

/* istanbul ignore if (react-native) */
if (preval`module.exports = process.env.BUILD_REACT_NATIVE === 'true'`) {
this.cleanup = () => {
Expand Down Expand Up @@ -983,6 +998,14 @@ class Downshift extends Component {
}

componentDidUpdate(prevProps, prevState) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we add this to a componentDidMount as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep that would make sense!

if (
this.getMenuProps.called &&
!this.getMenuProps.suppressRefError &&
/* istanbul ignore next (react-native) */ preval`module.exports = process.env.BUILD_REACT_NATIVE !== 'true'`
) {
validateGetMenuPropsCalledCorrectly(this._menuNode, this.getMenuProps)
}

if (
this.isControlledProp('selectedItem') &&
this.props.selectedItemChanged(
Expand Down Expand Up @@ -1031,6 +1054,10 @@ class Downshift extends Component {
this.getRootProps.called = false
this.getRootProps.refKey = undefined
this.getRootProps.suppressRefError = undefined
// we do something similar for getMenuProps
this.getMenuProps.called = false
this.getMenuProps.refKey = undefined
this.getMenuProps.suppressRefError = undefined
// we do something similar for getLabelProps
this.getLabelProps.called = false
// and something similar for getInputProps
Expand Down Expand Up @@ -1063,6 +1090,14 @@ class Downshift extends Component {

export default Downshift

function validateGetMenuPropsCalledCorrectly(node, {refKey}) {
if (!node) {
throw new Error(
`downshift: The ref prop "${refKey}" from getMenuProps was not applied correctly on your menu element.`,
)
}
}

function validateGetRootPropsCalledCorrectly(element, {refKey}) {
const refKeySpecified = refKey !== 'ref'
const isComposite = !isDOMElement(element)
Expand Down