-
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 3 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,53 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { Component, createRef } from '@wordpress/element'; | ||
import { focus } from '@wordpress/utils'; | ||
|
||
const withFocusContain = ( WrappedComponent ) => { | ||
return class extends Component { | ||
constructor() { | ||
super( ...arguments ); | ||
|
||
this.focusContainRef = createRef(); | ||
this.handleTabBehaviour = this.handleTabBehaviour.bind( this ); | ||
} | ||
|
||
handleTabBehaviour( event ) { | ||
if ( event.keyCode === 9 ) { | ||
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. Minor: Early return lets you avoid indenting the rest of the function, and I find to be generally more readable: if ( event.keyCode !== 9 ) {
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(); | ||
return lastTabbable.focus(); | ||
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. What/why are we |
||
} else if ( ! event.shiftKey && event.target === lastTabbable ) { | ||
event.preventDefault(); | ||
return firstTabbable.focus(); | ||
} | ||
} | ||
} | ||
|
||
componentDidMount() { | ||
this.focusContainRef.current.addEventListener( 'keydown', this.handleTabBehaviour ); | ||
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. Why do we need to bind this on the node directly instead of a prop Should we include a code comment informing future maintainers? |
||
} | ||
|
||
componentWillUnmount() { | ||
this.focusContainRef.current.addEventListener( 'keydown', this.handleTabBehaviour ); | ||
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.
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. See next comment. |
||
} | ||
|
||
render() { | ||
return ( | ||
<div ref={ this.focusContainRef }> | ||
<WrappedComponent { ...this.props } /> | ||
</div> | ||
); | ||
} | ||
}; | ||
}; | ||
|
||
export default withFocusContain; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
RangeControl | ||
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. Copy-pasta. 🍝 |
||
======= | ||
|
||
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. | ||
```js | ||
// When the app element is set it puts an aria-hidden="true" to the provided node. | ||
Modal.setAppElement( document.getElementById( 'wpwrap' ).parentNode ) | ||
``` | ||
```jsx | ||
<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> | ||
``` | ||
|
||
## 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,121 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import classnames from 'classnames'; | ||
import { noop } from 'lodash'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { Component, createPortal } from '@wordpress/element'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import ModalContent from './modal-content'; | ||
import * as ariaHelper from './aria-helper'; | ||
import './style.scss'; | ||
|
||
// Used to count the number of open modals. | ||
let modalCount = 0; | ||
|
||
let parentElement; | ||
|
||
class Modal extends Component { | ||
static setAppElement( node ) { | ||
ariaHelper.setAppElement( node ); | ||
} | ||
|
||
static setParentElement( node ) { | ||
if ( ! parentElement ) { | ||
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. Out of curiosity, why do we need Edit: I see this is to make it easier to exclude from applying |
||
parentElement = node; | ||
} | ||
} | ||
|
||
componentDidMount() { | ||
modalCount++; | ||
|
||
if ( ! this.parentElement ) { | ||
setElements(); | ||
} | ||
|
||
ariaHelper.hideApp(); | ||
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. Doing this on mount can be dangerous because it assumes developers will mount the modal properly. For example, assuming there's a controlling app with a state property everything works fine because the modal is not mounted on page load and the modal will be mounted (but not visible) on page load, and 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 fixed the logic behind this. |
||
parentElement.appendChild( this.node ); | ||
} | ||
|
||
componentWillUnmount() { | ||
modalCount--; | ||
|
||
if ( modalCount === 0 ) { | ||
ariaHelper.showApp(); | ||
} | ||
parentElement.removeChild( this.node ); | ||
} | ||
|
||
render() { | ||
const { | ||
overlayClassName, | ||
className, | ||
style: { | ||
content, | ||
overlay, | ||
}, | ||
children, | ||
...otherProps | ||
} = this.props; | ||
|
||
if ( ! this.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. This should be in the constructor. A |
||
this.node = document.createElement( 'div' ); | ||
} | ||
|
||
return createPortal( | ||
<div | ||
className={ classnames( | ||
'components-modal__screen-overlay', | ||
overlayClassName | ||
) } | ||
style={ overlay }> | ||
<ModalContent | ||
style={ content } | ||
className={ classnames( | ||
'components-modal__content', | ||
className | ||
) } | ||
{ ...otherProps } > | ||
{ children } | ||
</ModalContent> | ||
</div>, | ||
this.node | ||
); | ||
} | ||
} | ||
|
||
Modal.defaultProps = { | ||
className: null, | ||
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. What value is providing a default here doing? |
||
overlayClassName: null, | ||
onRequestClose: noop, | ||
focusOnMount: true, | ||
shouldCloseOnEsc: true, | ||
shouldCloseOnClickOutside: true, | ||
style: { | ||
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. Can we just remove this prop? I personally don't care if react-modal has certain functionality, if we don't need it or it doesn't fit our criteria for quality (encouraging good practices, etc), it shouldn't be included. 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 already removed it, but forgot to remove it from the defaultProps. |
||
content: null, | ||
overlay: null, | ||
}, | ||
/* accessibility */ | ||
contentLabel: null, | ||
aria: { | ||
labelledby: null, | ||
describedby: null, | ||
}, | ||
}; | ||
|
||
function setElements() { | ||
const wpwrapEl = document.getElementById( 'wpwrap' ); | ||
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 binds usage to a specific WordPress context, eliminating reusability of the component. The component shouldn't have any awareness of its ancestry. Can we pass this data via context instead? |
||
|
||
if ( wpwrapEl ) { | ||
Modal.setAppElement( wpwrapEl ); | ||
Modal.setParentElement( wpwrapEl.parentNode ); | ||
} | ||
} | ||
|
||
export default Modal; |
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.
There is a utility inside
@wordpress/utils
namedkeycodes
which will allow you to use a readable version of the keycode instead of using the number.