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

Add modal using react modal #6375

Closed
wants to merge 16 commits into from
Closed

Conversation

boblinthorst
Copy link
Contributor

@boblinthorst boblinthorst commented Apr 24, 2018

Description

Adds a modal component, making use of react-modal. The contents of the modal, and the opening and closing of the modal, is the responsibility of the implementor. An example of the implementation of this modal can be found in the test section.
An alternative solution to: #6261

How has this been tested?

To test the modal yourself, add the following to layout/index.js:

this one at the top of the file.

import ModalWrapper from '../modalWrapper';

And this one somewhere in the return

<ModalWrapper
	title="This is my modal"
	icon={ <span className="dashicons dashicons-admin-multisite"></span> }
	style={ {
		content: {
			width: '700px',
			height: '400px',
		},
	} }
>
	<p> super insteresting Content </p>
</ModalWrapper>

And create the file modalWrapper.js at components/modal, containing:

import Modal from '.';
import { Component } from '@wordpress/element';

class ModalWrapper extends Component {

	constructor( props ) {
		super( props );

		this.state = {
			isOpen: true,
		};

		this.onClose = this.onClose.bind( this );
	}

	onClose() {
		this.setState( {
			isOpen: false,
		} );
	}

	render() {
		const { children } = this.props;
		return <Modal { ...this.props } isOpen={ this.state.isOpen } onRequestClose={ this.onClose } >
			{ children }
		</Modal>;
	}
}

Screenshots

modal_screenshot

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@boblinthorst boblinthorst added the [Status] In Progress Tracking issues with work in progress label Apr 24, 2018
@boblinthorst boblinthorst changed the title add modal using react modal Add modal using react modal Apr 24, 2018
Copy link
Contributor

@abotteram abotteram left a comment

Choose a reason for hiding this comment

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

Only reviewed styling.

@@ -0,0 +1,105 @@
.edit-post-plugin-modal__editor-modal {
Copy link
Contributor

Choose a reason for hiding this comment

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

The prefix should be .components-modal according to the coding standards.
You could use the following structure for better readability:

.components-modal {
    &__screen-overlay {}
    &__content {}
    &__header {}
}

}
}

@include editor-left('.edit-post-plugin-screen-takeover__editor-screen-takeover-overlay');
Copy link
Contributor

Choose a reason for hiding this comment

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

screen takeover reference should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@include editor-left('.edit-post-plugin-screen-takeover__editor-screen-takeover-overlay');

// We prevent scrolling of the body when the React Modal screen takeover is opened.
body.ReactModal__Body--open {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use bodyOpenClassName props to overwrite ReactModal__Body--open.
While we're on the topic it would be favourable if portalClassName was also changed to something that is more suitable to WordPress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to WordPress-modal for now.

@IreneStr IreneStr added [Feature] Extensibility The ability to extend blocks or the editing experience and removed [Status] In Progress Tracking issues with work in progress labels Apr 25, 2018
@boblinthorst boblinthorst requested a review from aduth April 30, 2018 08:15
@aduth
Copy link
Member

aduth commented Apr 30, 2018

Can you elaborate on how this approach differs from #6261, in more detail than just that it uses react-modal. Do you think react-modal is a good fit, and in what ways? Will we be able to use it as a base for non-modal UIs like popovers and dropdown menus, or will we still need a custom implementation for those?

@aduth
Copy link
Member

aduth commented Jun 26, 2018

Are there plans to continue work here, or should this be closed in favor of #6261 ?

@aduth aduth added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jun 26, 2018
@boblinthorst
Copy link
Contributor Author

There are no plans to continue here. This was made as a comparison pr between #6261 and a react-modal solution. As this decision has already been made, this pr has served its purpose.

closed in favor of #6261

@gziolo gziolo deleted the try/add-modal-using-react-modal branch June 28, 2018 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants