-
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
Modal component #6261
Modal component #6261
Changes from 19 commits
bf9f287
aba9636
b0700fb
66367dd
aca49d0
3a5d75c
371e66b
bd48b5a
809bfdd
47219be
9b93633
de29a46
4a1b2c4
1092fbd
44d91d3
4aef9b6
5c0568c
887193d
b0c4d82
f25b989
c360742
984ef8e
6563c63
59255ff
c6ee481
17b7957
c4b433b
cca2a0e
21a722c
9313ecb
f487f22
a66f5b8
359774d
e7a8f4f
94a3216
064adbc
832b1ac
6ac30dd
349b068
d1d2ba6
eda32a6
dde71f2
ca8512a
afdaa2c
b6ef31f
b74d1e6
dbac197
61ac955
d94e4ef
6eb3886
ee6f520
ca59960
e2a50a1
010f223
9968113
5dc9b58
7f82944
146f562
30ab946
bd1050b
d5f50d1
2a0c916
9400adc
a956732
0679405
4e3bb1a
12dd5fe
f607330
40cc3f9
0590e39
6f57d08
c547404
4eba6c7
9ee5b83
1a0bf75
8778706
6a43d9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
/* eslint-disable jsx-a11y/no-static-element-interactions */ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { Component, createRef } from '@wordpress/element'; | ||
import { focus, keycodes } from '@wordpress/utils'; | ||
|
||
const { | ||
TAB, | ||
} = keycodes; | ||
|
||
const withFocusContain = ( WrappedComponent ) => { | ||
return class extends Component { | ||
constructor() { | ||
super( ...arguments ); | ||
|
||
this.focusContainRef = createRef(); | ||
this.handleTabBehaviour = this.handleTabBehaviour.bind( this ); | ||
} | ||
|
||
handleTabBehaviour( event ) { | ||
if ( ! event.keyCode === TAB ) { | ||
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. This is false for any key pressed, please double check. It should be 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. To clarify what's happening here, the negation occurs before the equality check, so this is effectively: if ( false === TAB ) { Makes me wish this ESLint rule were expanded to cover this case: https://eslint.org/docs/rules/no-unsafe-negation 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. No idea how I made this mistake... :/ |
||
return; | ||
} | ||
|
||
const tabbables = focus.tabbable.find( this.focusContainRef.current ); | ||
if ( ! tabbables.length ) { | ||
return; | ||
} | ||
const firstTabbable = tabbables[ 0 ]; | ||
const lastTabbable = tabbables[ tabbables.length - 1 ]; | ||
|
||
if ( event.shiftKey && event.target === firstTabbable ) { | ||
event.preventDefault(); | ||
lastTabbable.focus(); | ||
} else if ( ! event.shiftKey && event.target === lastTabbable ) { | ||
event.preventDefault(); | ||
firstTabbable.focus(); | ||
} | ||
} | ||
|
||
render() { | ||
return ( | ||
<div | ||
onKeyDown={ this.handleTabBehaviour } | ||
ref={ this.focusContainRef } > | ||
<WrappedComponent { ...this.props } /> | ||
</div> | ||
); | ||
} | ||
}; | ||
}; | ||
|
||
export default withFocusContain; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,14 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { forEach } from 'lodash'; | ||
import { forEach, noop } from 'lodash'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { | ||
Component, | ||
createRef, | ||
forwardRef, | ||
createHigherOrderComponent, | ||
} from '@wordpress/element'; | ||
|
||
|
@@ -26,13 +26,12 @@ const listener = new Listener(); | |
|
||
function withGlobalEvents( eventTypesToHandlers ) { | ||
return createHigherOrderComponent( ( WrappedComponent ) => { | ||
return class extends Component { | ||
class Wrapper extends Component { | ||
constructor() { | ||
super( ...arguments ); | ||
|
||
this.handleEvent = this.handleEvent.bind( this ); | ||
|
||
this.ref = createRef(); | ||
this.handleRef = this.handleRef.bind( this ); | ||
} | ||
|
||
componentDidMount() { | ||
|
@@ -49,15 +48,24 @@ function withGlobalEvents( eventTypesToHandlers ) { | |
|
||
handleEvent( event ) { | ||
const handler = eventTypesToHandlers[ event.type ]; | ||
if ( typeof this.ref.current[ handler ] === 'function' ) { | ||
this.ref.current[ handler ]( event ); | ||
if ( typeof this.wrappedRef[ handler ] === 'function' ) { | ||
this.wrappedRef[ handler ]( event ); | ||
} | ||
} | ||
|
||
handleRef( el ) { | ||
this.wrappedRef = el; | ||
this.props.forwardedRef( el ); | ||
} | ||
|
||
render() { | ||
return <WrappedComponent ref={ this.ref } { ...this.props } />; | ||
return <WrappedComponent { ...this.props } ref={ this.handleRef } />; | ||
} | ||
}; | ||
} | ||
|
||
return forwardRef( ( props, ref ) => { | ||
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. Should this be built-in to See similar mention at #6480 (comment) 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.
This comment has yet to be addressed or responded. 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.
For future readers, see related effort at #7557 . |
||
return <Wrapper { ...props } forwardedRef={ ref || noop } />; | ||
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. Is Do we need a fallback value? |
||
} ); | ||
}, 'withGlobalEvents' ); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
Modal | ||
======= | ||
|
||
The modal is used to create an accessible modal over an application. | ||
|
||
**Note:** The API for this modal has been mimicked to resemble `react-modal`. | ||
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. Should we link to Should we clarify whether the intent is for it to continue to strictly adhere to a compatible API into the future? 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. We've already diverged from the |
||
|
||
## Usage | ||
|
||
Render a screen overlay with a modal on top. | ||
```jsx | ||
<ModalContextProvider value={ { elementId: 'wpwrap' } }> | ||
<Modal | ||
aria={ { | ||
labelledby: 'modal-title', | ||
describedby: 'modal-description', | ||
} } | ||
parentSelector={ () => { | ||
return document.getElementById( 'wpwrap' ); | ||
} ) | ||
> | ||
<ModalContent> | ||
<h2 id="modal-title">My awesome modal!</h2> | ||
<p id="modal-description">This modal is meant to be awesome!</p> | ||
</ModalConent> | ||
</Modal> | ||
</ModalContextProvider> | ||
``` | ||
|
||
## Props | ||
|
||
The set of props accepted by the component will be specified below. | ||
Props not included in this set will be applied to the input elements. | ||
|
||
### onRequestClose | ||
|
||
This function is called to indicate that the modal should be closed. | ||
|
||
- Type: `function` | ||
- Required: Yes | ||
|
||
### contentLabel | ||
|
||
If this property is added, it will be added to the modal content `div` as `aria-label`. | ||
You are encouraged to use this if `aria.labelledby` is not provided. | ||
|
||
- Type: `String` | ||
- Required: No | ||
|
||
### aria.labelledby | ||
|
||
If this property is added, it will be added to the modal content `div` as `aria-labelledby`. | ||
You are encouraged to use this when the modal is visually labelled. | ||
|
||
- Type: `String` | ||
- Required: No | ||
|
||
### aria.describedby | ||
|
||
If this property is added, it will be added to the modal content `div` as `aria-describedby`. | ||
|
||
- Type: `String` | ||
- Required: No | ||
|
||
### focusOnMount | ||
|
||
If this property is true, it will focus the first tabbable element rendered in the modal. | ||
|
||
- Type: `bool` | ||
- Required: No | ||
- Default: true | ||
|
||
### shouldCloseOnEsc | ||
|
||
If this property is added, it will determine whether the modal requests to close when the escape key is pressed. | ||
|
||
- Type: `bool` | ||
- Required: No | ||
- Default: true | ||
|
||
### shouldCloseOnClickOutside | ||
|
||
If this property is added, it will determine whether the modal requests to close when a mouse click occurs outside of the modal content. | ||
|
||
- Type: `bool` | ||
- Required: No | ||
- Default: true | ||
|
||
### style.content | ||
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. For what purpose would someone be adding inline styles? Should we want to encourage this, vs. styling by an assigned class name? 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. Another case of trying to mimic the 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 share a similar concern, what would be the advantage of using inline styles over |
||
|
||
If this property is added, it will add inline styles to the modal content `div`. | ||
|
||
- Type: `Object` | ||
- Required: No | ||
|
||
### style.overlay | ||
|
||
If this property is added, it will add inline styles to the modal overlay `div`. | ||
|
||
- Type: `Object` | ||
- Required: No | ||
|
||
### className | ||
|
||
If this property is added, it will an additional class name to the modal content `div`. | ||
|
||
- Type: `String` | ||
- Required: No | ||
|
||
### overlayClassName | ||
|
||
If this property is added, it will an additional class name to the modal overlay `div`. | ||
|
||
- Type: `String` | ||
- Required: No |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
let appElement = null; | ||
|
||
export function setAppElement( node ) { | ||
if ( ! appElement ) { | ||
appElement = node; | ||
} | ||
} | ||
|
||
export function hideApp() { | ||
if ( appElement ) { | ||
appElement.setAttribute( 'aria-hidden', 'true' ); | ||
} | ||
} | ||
|
||
export function showApp() { | ||
if ( appElement ) { | ||
appElement.removeAttribute( 'aria-hidden' ); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { createContext, createHigherOrderComponent } from '@wordpress/element'; | ||
|
||
const { Consumer, Provider } = createContext( { | ||
appElementId: null, | ||
} ); | ||
|
||
export { Provider as ModalContextProvider }; | ||
|
||
/** | ||
* A Higher-order Component used to inject Modal context into the wrapped | ||
* component. | ||
* | ||
* @param {Component} OriginalComponent Component to wrap. | ||
* | ||
* @return {Component} Component with Modal context injected. | ||
*/ | ||
export const withModalContext = createHigherOrderComponent( | ||
( OriginalComponent ) => ( props ) => ( | ||
<Consumer> | ||
{ ( modalContext ) => ( | ||
<OriginalComponent | ||
{ ...props } | ||
modalContext={ modalContext } | ||
/> | ||
) } | ||
</Consumer> | ||
), | ||
'withModalContext' | ||
); |
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.
We should almost never want to disable a rule for an entire file, since while it may be valid to disable it for the specific usage in place now, file-wide disabling will take effect for any future additions which themselves should be subject to the same scrutiny as the original rule intends.
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.
Please also consider there's #6987 proposing to implement this HoC separately.