-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add a useDialog hook and replace the duplicated PopoverWrapper #27643
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
`useDialog` | ||
=========== | ||
|
||
React hook to be used on a dialog wrapper to enable the following behaviors: | ||
|
||
- constrained tabbing. | ||
- focus on mount. | ||
- return focus on unmount. | ||
- focus outside. | ||
|
||
## Returned value | ||
|
||
The hooks returns an array composed of the two following values: | ||
|
||
### `ref` | ||
|
||
- Type: `Object|Function` | ||
|
||
A React ref that must be passed to the DOM element where the behavior should be attached. | ||
|
||
### `props` | ||
|
||
- Type: `Object` | ||
|
||
Extra props to apply to the wrapper. | ||
|
||
## Usage | ||
|
||
```jsx | ||
import { __experimentalUseDialog as useDialog } from '@wordpress/compose'; | ||
|
||
const MyDialog = () => { | ||
const [ ref, extraProps ] = useDialog( { | ||
onClose: () => console.log('do something to close the dialog') | ||
} ); | ||
|
||
return ( | ||
<div ref={ ref } {...extraProps}> | ||
<Button /> | ||
<Button /> | ||
</div> | ||
); | ||
}; | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import mergeRefs from 'react-merge-refs'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { useRef, useCallback, useEffect } from '@wordpress/element'; | ||
import { ESCAPE } from '@wordpress/keycodes'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import useConstrainedTabbing from '../use-constrained-tabbing'; | ||
import useFocusOnMount from '../use-focus-on-mount'; | ||
import useFocusReturn from '../use-focus-return'; | ||
import useFocusOutside from '../use-focus-outside'; | ||
|
||
/** | ||
* Returns a ref and props to apply to a dialog wrapper to enable the following behaviors: | ||
* - constrained tabbing. | ||
* - focus on mount. | ||
* - return focus on unmount. | ||
* - focus outside. | ||
* | ||
* @param {Object} options Dialog Options. | ||
*/ | ||
function useDialog( options ) { | ||
const onClose = useRef(); | ||
useEffect( () => { | ||
onClose.current = options.onClose; | ||
}, [ options.onClose ] ); | ||
const constrainedTabbingRef = useConstrainedTabbing(); | ||
const focusOnMountRef = useFocusOnMount(); | ||
const focusReturnRef = useFocusReturn(); | ||
const focusOutsideProps = useFocusOutside( options.onClose ); | ||
const closeOnEscapeRef = useCallback( ( node ) => { | ||
if ( ! node ) { | ||
return; | ||
} | ||
|
||
node.addEventListener( 'keydown', ( event ) => { | ||
// Close on escape | ||
if ( event.keyCode === ESCAPE && onClose.current ) { | ||
event.stopPropagation(); | ||
onClose.current(); | ||
} | ||
} ); | ||
|
||
node.addEventListener( 'mousedown', ( event ) => { | ||
// I'm not really certain what this is for but it matches the previous behavior. | ||
ntsekouras marked this conversation as resolved.
Show resolved
Hide resolved
|
||
event.stopPropagation(); | ||
} ); | ||
}, [] ); | ||
|
||
return [ | ||
mergeRefs( [ | ||
constrainedTabbingRef, | ||
focusOnMountRef, | ||
focusReturnRef, | ||
closeOnEscapeRef, | ||
] ), | ||
focusOutsideProps, | ||
]; | ||
} | ||
|
||
export default useDialog; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
* WordPress dependencies | ||
*/ | ||
import { focus } from '@wordpress/dom'; | ||
import { useEffect, useRef } from '@wordpress/element'; | ||
import { useCallback, useEffect, useRef, useState } from '@wordpress/element'; | ||
|
||
/** | ||
* Hook used to focus the first tabbable element on mount. | ||
|
@@ -27,48 +27,54 @@ import { useEffect, useRef } from '@wordpress/element'; | |
*/ | ||
function useFocusOnMount( focusOnMount = 'firstElement' ) { | ||
const focusOnMountRef = useRef( focusOnMount ); | ||
const [ node, setNode ] = useState(); | ||
useEffect( () => { | ||
focusOnMountRef.current = focusOnMount; | ||
}, [ focusOnMount ] ); | ||
|
||
// Ideally we should be using a function ref (useCallback) | ||
// Ideally we should be running the focus behavior in the useCallback directly | ||
// Right now we have some issues where the link popover remounts | ||
// prevents us from doing that. | ||
// The downside of the current implementation is that it doesn't update if the "ref" changes. | ||
const ref = useRef(); | ||
const ref = useCallback( setNode, [] ); | ||
|
||
// Focus handling | ||
useEffect( () => { | ||
if ( ! node ) { | ||
return; | ||
} | ||
/* | ||
* Without the setTimeout, the dom node is not being focused. Related: | ||
* https://stackoverflow.com/questions/35522220/react-ref-with-focus-doesnt-work-without-settimeout-my-example | ||
* | ||
* TODO: Treat the cause, not the symptom. | ||
*/ | ||
const focusTimeout = setTimeout( () => { | ||
if ( ! focusOnMountRef.current || ! ref.current ) { | ||
if ( focusOnMountRef.current === false || ! node ) { | ||
ntsekouras marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return; | ||
} | ||
|
||
if ( focusOnMountRef.current === 'firstElement' ) { | ||
const firstTabbable = focus.tabbable.find( ref.current )[ 0 ]; | ||
const firstTabbable = focus.tabbable.find( node )[ 0 ]; | ||
|
||
if ( firstTabbable ) { | ||
firstTabbable.focus(); | ||
} else { | ||
ref.current.focus(); | ||
node.focus(); | ||
} | ||
|
||
return; | ||
} | ||
|
||
if ( focusOnMountRef.current === 'container' ) { | ||
ref.current.focus(); | ||
if ( | ||
focusOnMountRef.current === 'container' || | ||
focusOnMountRef.current === true | ||
) { | ||
node.focus(); | ||
} | ||
}, 0 ); | ||
|
||
return () => clearTimeout( focusTimeout ); | ||
}, [] ); | ||
}, [ node ] ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you add a dependency here, the promise of only focussing on mount is broken. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so, I'm focusing only when the "node" is mounted, not the component. |
||
|
||
return ref; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is ref separate? Ref is also a prop that can be spread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's still subject to change, my hope is that we can remove the props entirely and just return the ref. more explanations here #27643 (comment)