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

Modal component #6261

Merged
merged 77 commits into from
Jul 4, 2018
Merged
Show file tree
Hide file tree
Changes from 56 commits
Commits
Show all changes
77 commits
Select commit Hold shift + click to select a range
bf9f287
Initial implementation modal
Apr 18, 2018
aba9636
removed style prop assignment causing error
Apr 19, 2018
b0700fb
Set default mount node to #wpwrap
Apr 23, 2018
66367dd
Implemented default styling
Apr 25, 2018
aca49d0
Improved styling
Apr 25, 2018
3a5d75c
Applied code review feedback to with-focus-contain HOC
Apr 25, 2018
371e66b
Added eslint ignore for jsx-a11y/no-static-element-interactions
Apr 25, 2018
bd48b5a
Implemented withGlobalEvents HOC
Apr 25, 2018
809bfdd
withGlobalEvents HOC now forwards refs
Apr 30, 2018
47219be
Replace lodash defer with withSafeTimeout
Apr 30, 2018
9b93633
Removed unnecessary return statements
Apr 30, 2018
de29a46
Created separate styling rules
May 1, 2018
4a1b2c4
Added documentation for forwardRef function
May 1, 2018
1092fbd
Made mount location for modal configurable
May 1, 2018
44d91d3
Renamed elementId to appElementId for clarity
May 1, 2018
4aef9b6
Added noop for when no reg is provided in forwardRef
May 1, 2018
5c0568c
Fixed error in EditorProvider
May 1, 2018
887193d
Fix eslint errors
May 1, 2018
b0c4d82
Fixed incorrectly bound function
May 1, 2018
f25b989
Modal now by default mounts to the body and hides all other elements
May 2, 2018
c360742
hideApp no longer unhides elements that already had a aria-hidden=tru…
May 2, 2018
984ef8e
Improved a11y and updated documentation
May 2, 2018
6563c63
Updated documentation
May 2, 2018
59255ff
Removed forwardRef from element|
May 2, 2018
c6ee481
Changed default close label to Close dialog
May 2, 2018
17b7957
Add modal-open className to body when modal is opened
May 2, 2018
c4b433b
Added documentation to modal/index.js
May 2, 2018
cca2a0e
Removed aria-modal=true and explained why in aria-helper.js
May 2, 2018
21a722c
Documented modal/frame.js
May 2, 2018
9313ecb
Merge branch 'master' into add/modal
xyfi May 8, 2018
f487f22
Merge branch 'master' into add/modal
atimmer May 23, 2018
a66f5b8
Merge branch 'master' into add/modal
abotteram May 29, 2018
359774d
Merge branch 'add/modal' of https://github.com/WordPress/gutenberg in…
abotteram May 29, 2018
e7a8f4f
Addressed eslint issues
abotteram May 29, 2018
94a3216
Polish the visuals a bit.
Jun 6, 2018
064adbc
Merge branch 'master' into add/modal
abotteram Jun 7, 2018
832b1ac
Merge branch 'add/modal' of https://github.com/WordPress/gutenberg in…
abotteram Jun 7, 2018
6ac30dd
Disabled jsx-a11y/no-static-element-interactions in render function o…
abotteram Jun 7, 2018
349b068
Merge branch 'master' into add/modal
abotteram Jun 13, 2018
d1d2ba6
Merge branch 'master' into add/modal
abotteram Jun 18, 2018
eda32a6
Addressed CR concerns
abotteram Jun 18, 2018
dde71f2
Removed unused variable
abotteram Jun 18, 2018
ca8512a
Replaced focus.tabbables.find from @wordpress/utils with @wordpress/dom
abotteram Jun 18, 2018
afdaa2c
Merge branch 'master' into add/modal
abotteram Jun 20, 2018
b6ef31f
Fixed failing tests after updating react-test-renderer to version 16.…
abotteram Jun 20, 2018
b74d1e6
CSS Tweaks
abotteram Jun 21, 2018
dbac197
Fixed error when clicking outside of the modal
abotteram Jun 26, 2018
61ac955
Move focus to first element with tabindex=-1 on mount
abotteram Jun 26, 2018
d94e4ef
Make sure the dic the modal is renderd in is apprended to the documen…
abotteram Jun 27, 2018
6eb3886
Addressed minor codestyle issues in frame.js
abotteram Jun 27, 2018
ee6f520
Fixed bug when opening modal the second time
abotteram Jun 27, 2018
ca59960
Removed unused import
abotteram Jun 27, 2018
e2a50a1
replaced react-click-outside with internal withFocusOutside HOC
abotteram Jun 27, 2018
010f223
Merge branch 'master' into add/modal
abotteram Jul 2, 2018
9968113
Replaced withFocusContain with withConstrainedTabbing
abotteram Jul 2, 2018
5dc9b58
Replaced withFocusOutside with react-click-outside again
abotteram Jul 2, 2018
7f82944
Replaced @wordpress/utils keycodes with @wordpress/keycodes in frame.js
abotteram Jul 3, 2018
146f562
don't pass props.aria.labelledby to frame div when props.contentLabel…
abotteram Jul 3, 2018
30ab946
Added logic and tests for aria-helper to not hide (implicitely) live …
abotteram Jul 3, 2018
bd1050b
Removed isOpen props from the modal
abotteram Jul 3, 2018
d5f50d1
Removed the ability to add inline styles to the modal
abotteram Jul 3, 2018
2a0c916
Removed useless css
abotteram Jul 3, 2018
9400adc
Removed redundant z-index
abotteram Jul 3, 2018
a956732
Add full page overlay
abotteram Jul 3, 2018
0679405
Don't render h1 tag when no title is provided
abotteram Jul 3, 2018
4e3bb1a
Made modal screen-verlay full screen
abotteram Jul 3, 2018
12dd5fe
generate unique id for modal labelledby attribute
abotteram Jul 3, 2018
f607330
Replaced function scoped array liveRegionAriaRoles with file scoped c…
abotteram Jul 3, 2018
40cc3f9
Removed check whtehr forwardedRef is defined
abotteram Jul 3, 2018
0590e39
Minor JSDoc improvement
abotteram Jul 3, 2018
6f57d08
Removed styles from defaultProps
abotteram Jul 3, 2018
c547404
Documentation improvements
abotteram Jul 3, 2018
4eba6c7
don't add labelledBy attribute to modal frame when no title is present
abotteram Jul 3, 2018
9ee5b83
Don't add unique id to headingId when aria.labelledby prop is provide…
abotteram Jul 3, 2018
1a0bf75
Components: Reorder component lifecycle as first class members
aduth Jul 4, 2018
8778706
Components: Use withInstanceId to generate modal heading id
aduth Jul 4, 2018
6a43d9f
Components: Fix modal withInstanceId import reference
aduth Jul 4, 2018
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
26 changes: 18 additions & 8 deletions components/higher-order/with-global-events/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { forEach } from 'lodash';
*/
import {
Component,
createRef,
forwardRef,
createHigherOrderComponent,
} from '@wordpress/element';

Expand All @@ -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() {
Expand All @@ -49,15 +48,26 @@ 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;
if ( this.props.forwardedRef ) {
Copy link
Member

Choose a reason for hiding this comment

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

When will this condition not be satisfied?

Copy link
Contributor Author

@abotteram abotteram Jun 27, 2018

Choose a reason for hiding this comment

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

For some reason when react-test-renderer is used this condition is not satisfied. I know we shouldn't implement logic to satisfy tests, but this was a quick and easy fix.

Copy link
Member

Choose a reason for hiding this comment

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

I know we shouldn't implement logic to satisfy tests, but this was a quick and easy fix.

Please make an issue to correct this, ideally self-assigned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been fixed.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's been fixed; this line is causing #7707 and adding the check back prevents the error.

Though it would indicate these refs aren't being forwarded at all.

this.props.forwardedRef( el );
}
}

render() {
return <WrappedComponent ref={ this.ref } { ...this.props } />;
return <WrappedComponent { ...this.props } ref={ this.handleRef } />;
}
};
}

return forwardRef( ( props, ref ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be built-in to createHigherOrderComponent ?

See similar mention at #6480 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Should this be built-in to createHigherOrderComponent ?

See similar mention at #6480 (comment)

This comment has yet to be addressed or responded.

Copy link
Member

Choose a reason for hiding this comment

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

Should this be built-in to createHigherOrderComponent ?

See similar mention at #6480 (comment)

This comment has yet to be addressed or responded.

For future readers, see related effort at #7557 .

return <Wrapper { ...props } forwardedRef={ ref } />;
} );
}, 'withGlobalEvents' );
}

Expand Down
12 changes: 7 additions & 5 deletions components/higher-order/with-global-events/test/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { mount } from 'enzyme';
import TestRenderer from 'react-test-renderer';

/**
* External dependencies
Expand Down Expand Up @@ -62,20 +62,22 @@ describe( 'withGlobalEvents', () => {
resize: 'handleResize',
} )( OriginalComponent );

wrapper = mount( <EnhancedComponent { ...props }>Hello</EnhancedComponent> );
wrapper = TestRenderer.create( <EnhancedComponent { ...props }>Hello</EnhancedComponent> );
}

it( 'renders with original component', () => {
mountEnhancedComponent();

expect( wrapper.childAt( 0 ).childAt( 0 ).type() ).toBe( 'div' );
expect( wrapper.childAt( 0 ).text() ).toBe( 'Hello' );
expect( wrapper.root.findByType( 'div' ).children[ 0 ] ).toBe( 'Hello' );
} );

it( 'binds events from passed object', () => {
mountEnhancedComponent();

expect( Listener._instance.add ).toHaveBeenCalledWith( 'resize', wrapper.instance() );
// Get the HOC wrapper instance
const hocInstance = wrapper.root.findByType( OriginalComponent ).parent.instance;

expect( Listener._instance.add ).toHaveBeenCalledWith( 'resize', hocInstance );
} );

it( 'handles events', () => {
Expand Down
1 change: 1 addition & 0 deletions components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export { default as KeyboardShortcuts } from './keyboard-shortcuts';
export { default as MenuGroup } from './menu-group';
export { default as MenuItem } from './menu-item';
export { default as MenuItemsChoice } from './menu-items-choice';
export { default as Modal } from './modal';
export { default as ScrollLock } from './scroll-lock';
export { NavigableMenu, TabbableContainer } from './navigable-container';
export { default as Notice } from './notice';
Expand Down
176 changes: 176 additions & 0 deletions components/modal/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
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`](https://github.com/reactjs/react-modal).

## Usage

Render a screen overlay with a modal on top.
```jsx
<Modal
title="My Modal"
onRequestClose={ closeFunction }
isOpen={ openState }
Copy link
Member

Choose a reason for hiding this comment

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

This example is out of date. The isOpen prop no longer exists.

aria={ {
describedby: "modal-description",
} }
>
<ModalContent>
<p id="modal-description">This modal is meant to be awesome!</p>
Copy link
Member

Choose a reason for hiding this comment

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

We're encouraging a bad practice here with applying a static id to an element, one which from what I can tell doesn't serve any purpose anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the aria.describedby to the example to demonstrate how this prop can be used to improve accessibility.

Copy link
Member

Choose a reason for hiding this comment

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

We still should never encourage / allow static id. We should update the example to use withInstanceId to guarantee a unique ID if we want the ID to be included.

https://github.com/WordPress/gutenberg/tree/master/components/higher-order/with-instance-id

Alternatively, since there's decent overhead in doing so, maybe this is something we should bake as behavior on behalf of the developer?

</ModalConent>
</Modal>
```

## Implement close logic

For the modal to properly work it's important you implement the close logic for the modal properly. The following example shows you how to properly implement a modal.

```js
const { Component, Fragment } = wp.element;
const { Modal } = wp.components;

class MyModalWrapper extends Component {
constructor() {
super( ...arguments );
this.state = {
isOpen: true,
}

this.openModal = this.openModal.bind( this );
this.closeModal = this.closeModal.bind( this );
}

openModal() {
if ( ! this.state.isOpen ) {
this.setState( { isOpen: true } );
}
}

closeModal() {
if ( this.state.isOpen ) {
this.setState( { isOpen: false } );
}
}

render() {
return (
<Fragment>
<button onClick={ this.openModal }>Open Modal</button>
<Modal
title="This is my modal"
onRequestClose={ this.closeModal }
isOpen={ this.state.isOpen }>
Copy link
Member

Choose a reason for hiding this comment

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

This is something we used to have with Popover, and then was later removed in #5015 because there's a performance impact in allowing the element and its children to be created wastefully (with no intention of ever rendering except by basis of boolean value). Further, there's a point toward consistency between usage of the two components.

Would it be possible to remove this boolean and have the modal render simply by its presence?

Copy link
Member

Choose a reason for hiding this comment

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

Also, this isOpen prop is not documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is possible to omit the open prop. It was just an attempt to keep true to the react-modal API, but I think we should not keep trying to mimic it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I was about to propose the same given that this component is useless when isOpen flag is set to false.

<button onClick={ this.closeModal }>
My custom close button
</button>
</Modal>
</Fragment>
);
}
}
```

## 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.

### title

This property is used as the modal header's title. It is required for accessibility reasons.

- Type: `String`
- Required: Yes

### 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
- Default: `modal-heading`

### 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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another case of trying to mimic the react-modal API.

Copy link
Member

Choose a reason for hiding this comment

The 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 className which is also listed as one of the props available.


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

### role

If this property is added, it will override the default role of the modal.

- Type: `String`
- Required: No
- Default: `dialog`

### overlayClassName

If this property is added, it will an additional class name to the modal overlay `div`.

- Type: `String`
- Required: No
53 changes: 53 additions & 0 deletions components/modal/aria-helper.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/**
* External dependencies
*/
import { forEach } from 'lodash';

let hiddenElements = [],
isHidden = false;

/**
* Hides all elements in the body element from screen-readers except
* the provided element, script elements and elements that already have
* an `aria-hidden="true"` attribute.
*
* The reason we do this is because `aria-modal="true"` currently is bugged
* in Safari, and support is spotty in other browsers overall. In the future
* we should consider removing these helper functions in favor of
* `aria-modal="true"`.
*
* @param {Element} unhiddenElement The element that should not be hidden.
*/
export function hideApp( unhiddenElement ) {
if ( isHidden ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is the condition (and the associated isHidden variable) needed? Are we expecting this function to be called excessively often so as to need the optimization shortcut? Would it break if we otherwise allow the function to proceed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be called multiple time in case there are multiple modals. Although this is not desired, various plugins could open modals making this check a worthwhile check in my opinion.

return;
}
const elements = document.body.children;
forEach( elements, ( element ) => {
if (
element === unhiddenElement ||
element.tagName === 'SCRIPT' ||
Copy link
Member

Choose a reason for hiding this comment

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

On what basis did you decide script (and not any other node types) should be exempt from aria-hidden ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a simple DOM inspection after running this function without any additional checks, and having script tags with aria-hidden="true" seemed unnecessary, so to limit the amount of DOM operations I decided script tags could be excluded from this operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afercia Commented: Well <script> elements are not content, they don’t need to be hidden. Also, many of them contain templates for the media UI so better not touch them?

element.hasAttribute( 'aria-hidden', 'true' )
) {
return;
}
element.setAttribute( 'aria-hidden', 'true' );
hiddenElements.push( element );
} );
isHidden = true;
}

/**
* Makes all elements in the body that have been hidden by `hideApp`
* visible again to screen-readers.
*/
export function showApp() {
if ( ! isHidden ) {
return;
}
forEach( hiddenElements, ( element ) => {
element.removeAttribute( 'aria-hidden' );
} );
hiddenElements = [];
isHidden = false;
}
Loading